Re: [PATCH 1/2] Allow move of compiled_regex

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tromey@adacore.com> writes:

> This patch makes it possible to move a compiled_regex.
> ---
>  gdbsupport/gdb_regex.cc | 13 +++++++++++--
>  gdbsupport/gdb_regex.h  | 30 +++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/gdbsupport/gdb_regex.cc b/gdbsupport/gdb_regex.cc
> index 111e5ff24a8..6774aa4c805 100644
> --- a/gdbsupport/gdb_regex.cc
> +++ b/gdbsupport/gdb_regex.cc
> @@ -33,17 +33,25 @@ compiled_regex::compiled_regex (const char *regex, int cflags,
>        regerror (code, &m_pattern, err.data (), length);
>        error (("%s: %s"), message, err.data ());
>      }
> +
> +  m_valid = true;
>  }
>  
> -compiled_regex::~compiled_regex ()
> +void
> +compiled_regex::clear ()
>  {
> -  regfree (&m_pattern);
> +  if (m_valid)
> +    {
> +      regfree (&m_pattern);
> +      m_valid = false;
> +    }
>  }
>  
>  int
>  compiled_regex::exec (const char *string, size_t nmatch,
>  		      regmatch_t pmatch[], int eflags) const
>  {
> +  gdb_assert (m_valid);
>    return regexec (&m_pattern, string, nmatch, pmatch, eflags);
>  }
>  
> @@ -52,5 +60,6 @@ compiled_regex::search (const char *string,
>  			int length, int start, int range,
>  			struct re_registers *regs)
>  {
> +  gdb_assert (m_valid);
>    return re_search (&m_pattern, string, length, start, range, regs);
>  }
> diff --git a/gdbsupport/gdb_regex.h b/gdbsupport/gdb_regex.h
> index 63fd9c32119..f7d3190839f 100644
> --- a/gdbsupport/gdb_regex.h
> +++ b/gdbsupport/gdb_regex.h
> @@ -35,10 +35,32 @@ class compiled_regex

I think the comment above the compiled_regex class needs to be updated.
It currently says:

  /* A compiled regex.  This is mainly a wrapper around regex_t.  The
     the constructor throws on regcomp error and the destructor is
     responsible for calling regfree.  The former means that it's not
     possible to create an instance of compiled_regex that isn't
     compiled, hence the name.  */

[ NOTE: There's a pre-existing typo: "The the constructor throws ...". ]

The sentence: "The former means that it's not possible to create an
instance of compiled_regex that isn't compiled, hence the name." is now
out of date I think.  Previously, if I did:

  compiled_regex re ( ... args ...);

then RE was either (a) valid, or (b) an exception was thrown.  However,
this is no longer the case:

  compiled_regex re ( ... args ...);
  compiled_regex re2 (std::move (re));
  /* Now RE is in a non-compiled state.  */

So the comment needs to be updated to indicate that a compiled_regexp is
no longer always compiled.

With a suitable comment in place:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>  		  const char *message)
>      ATTRIBUTE_NONNULL (2) ATTRIBUTE_NONNULL (4);
>  
> -  ~compiled_regex ();
> +  ~compiled_regex ()
> +  {
> +    clear ();
> +  }
>  
>    DISABLE_COPY_AND_ASSIGN (compiled_regex);
>  
> +  compiled_regex (compiled_regex &&other)
> +    : m_valid (other.m_valid),
> +      m_pattern (other.m_pattern)
> +  {
> +    other.m_valid = false;
> +  }
> +
> +  compiled_regex &operator= (compiled_regex &&other)
> +  {
> +    if (&other != this)
> +      {
> +	clear ();
> +	m_valid = other.m_valid;
> +	m_pattern = other.m_pattern;
> +	other.m_valid = false;
> +      }
> +    return *this;
> +  }
> +
>    /* Wrapper around ::regexec.  */
>    int exec (const char *string,
>  	    size_t nmatch, regmatch_t pmatch[],
> @@ -50,6 +72,12 @@ class compiled_regex
>  	      int range, struct re_registers *regs);
>  
>  private:
> +  /* Free the compiled pattern, if necessary.  */
> +  void clear ();
> +
> +  /* True if the compiled pattern is valid.  */
> +  bool m_valid = false;
> +
>    /* The compiled pattern.  */
>    regex_t m_pattern;
>  };
>
> -- 
> 2.53.0