1/2: D66174 `RegularExpression` cleanup

[jankratochvil]

Summary

I find as a good cleanup to drop the Compile method. As I do not find TIMTOWTDI as an advantage and there is already constructor parameter to compile the regex.
If this is not considered as a cleanup I will drop this patch and rework D66398 to use the Compile method again.

[JDevlieghere]

This code review seems to address a few things. I think it might have been easier to discuss if they were separate patches, but here are my comments:

• Thanks for removing the stale inheritance! (LGTM)
• Thanks for adding the relational operator back. I couldn't find why they were needed (because they weren't used if I understand D66398 correctly). (LGTM)
• While I was doing the transition I considered the lifetime and came to the conclusion that the regex input string (the StringRef) does not need to outlive the regex compilation (regcomp). The interface of llvm::Regex gives the same impression. Is this not correct?
• I'm not a fan of the laziness. Do we really need it? What's the cost of compiling an empty regex for the default constructed object? Now we have to (lazily) recompile the regex after every move, even though an llvm::Regex was designed to be movable.

lldb/include/lldb/Utility/RegularExpression.h

86

This doesn't sound right, we return the error by value and llvm::StringError stores the error string.

[jankratochvil]

In D66392#1634575, @JDevlieghere wrote:

• Thanks for removing the stale inheritance! (LGTM)

Checked it in now as rL369235.

• Thanks for adding the relational operator back. I couldn't find why they were needed (because they weren't used if I understand D66398 correctly). (LGTM)

Yes, you are right the operators were not used. So I have also moved them now to D66398 from this patch.

• While I was doing the transition I considered the lifetime and came to the conclusion that the regex input string (the StringRef) does not need to outlive the regex compilation (regcomp). The interface of llvm::Regex gives the same impression. Is this not correct?

OK, it is correct. So I dropped the core of this patch (+added some lifetime explanatory comments).

• I'm not a fan of the laziness. Do we really need it?

That is no longer a part of this patch.
Just then I find as a good cleanup to drop the Compile method there, don't you? As I do not find TIMTOWTDI as an advantage and there is already constructor parameter to compile the regex.
If this is not considered as a cleanup I will drop this patch and rework D66398 to use the Compile method again.
I am aware the llvm/ part of this patch needs an extra approval and this is why @JDevlieghere had there that:

std::string discarded;
return m_regex.isValid(discarded);

Is it complicated to get the llvm/ extension approved? I can quickly drop any parts of this patch not considered as feasible.

[jankratochvil]

In D66392#1634575, @JDevlieghere wrote:

I am aware the llvm/ part of this patch needs an extra approval and this is why @JDevlieghere had there that:

std::string discarded;
return m_regex.isValid(discarded);

Is it complicated to get the llvm/ extension approved? I can quickly drop any parts of this patch not considered as feasible.

I have put there plain m_regex.isValid() instead as with my patch IsValid() is called during every Execute() so IsValid() could become a bottleneck. But then the discarded string is just 32 bytes on stack anyway so it should be cheap enough even without the llvm/ extension.

[jankratochvil]

(comment typo fixed)

[labath]

I agree that it is better to have just one way to initialize the regex object. This looks good to me modulo the llvm parts. I'd suggest just dropping those, or creating a separate patch for that...

lldb/include/lldb/Interpreter/OptionValueRegex.h

53–55

The apparent inconsistency here is a pretty good argument against the Compile method imo.

lldb/include/lldb/Utility/RegularExpression.h

34

Null terminated? I hope not..

lldb/source/Commands/CommandObjectFrame.cpp

536–537

lol

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

445

Here I'd just declare a fresh variable in each of the blocks instead of continuously recycling this one.

lldb/source/Target/ThreadPlanStepInRange.cpp

317–320

maybe swap the then and else blocks around to reduce the amount of negation

lldb/source/Utility/RegularExpression.cpp

16–20

Initialize this stuff in the member initializer list?

[JDevlieghere]

Thanks, Jan. This LGTM with Pavel's comments addressed.

[jankratochvil]

lldb/include/lldb/Utility/RegularExpression.h

34

Updated+added:

+  ///     An llvm::StringRef that represents the regular expression to compile.
+  //      String is not referenced anymore after the object is constructed.

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

445

Rather dropped the variable.

[jankratochvil]

Thanks for the review, I hope my updates for the check-in are fine.

lldb/include/lldb/Interpreter/OptionValueRegex.h

public:
 
   // Subclass specific functions
   const RegularExpression *GetCurrentValue() const {
     return (m_regex.IsValid() ? &m_regex : nullptr);
   }
 
   void SetCurrentValue(const char *value) {
     if (value && value[0])
-      m_regex.Compile(llvm::StringRef(value));
+      m_regex = RegularExpression(llvm::StringRef(value));
     else
       m_regex = RegularExpression();
   }
 
   bool IsValid() const { return m_regex.IsValid(); }
 
 protected:
   RegularExpression m_regex;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_OptionValueRegex_h_

lldb/include/lldb/Utility/RegularExpression.h

 class RegularExpression {
 public:
   /// Default constructor.
   ///
   /// The default constructor that initializes the object state such that it
   /// contains no compiled regular expression.
   RegularExpression() = default;
 
+  /// Constructor for a regular expression.
+  ///
+  /// Compile a regular expression using the supplied regular expression text.
+  /// The compiled regular expression lives in this object so that it can be
+  /// readily used for regular expression matches. Execute() can be called
+  /// after the regular expression is compiled.
+  ///
+  /// \param[in] string
+  ///     A NULL terminated C string that represents the regular
+  ///     expression to compile.
   explicit RegularExpression(llvm::StringRef string);
+
   ~RegularExpression() = default;
 
   RegularExpression(const RegularExpression &rhs);
   RegularExpression(RegularExpression &&rhs) = default;
 
   RegularExpression &operator=(RegularExpression &&rhs) = default;
   RegularExpression &operator=(const RegularExpression &rhs) = default;
 
-  /// Compile a regular expression.
-  ///
-  /// Compile a regular expression using the supplied regular expression text.
-  /// The compiled regular expression lives in this object so that it can be
-  /// readily used for regular expression matches. Execute() can be called
-  /// after the regular expression is compiled. Any previously compiled
-  /// regular expression contained in this object will be freed.
-  ///
-  /// \param[in] re
-  ///     A NULL terminated C string that represents the regular
-  ///     expression to compile.
-  ///
-  /// \return \b true if the regular expression compiles successfully, \b false
-  ///     otherwise.
-  bool Compile(llvm::StringRef string);
-
   /// Executes a regular expression.
   ///
   /// Execute a regular expression match using the compiled regular expression
   /// that is already in this object against the match string \a s. If any
   /// parens are used for regular expression matches \a match_count should
   /// indicate the number of regmatch_t values that are present in \a
   /// match_ptr.
   ///
   /// \param[in] string
   ///     The string to match against the compile regular expression.
   ///
   /// \param[in] match
   ///     A pointer to a RegularExpression::Match structure that was
   ///     properly initialized with the desired number of maximum
   ///     matches, or nullptr if no parenthesized matching is needed.
   ///
   /// \return \b true if \a string matches the compiled regular expression, \b
-  ///     false otherwise.
+  ///     false otherwise incl. the case regular exression failed to compile.
   bool Execute(llvm::StringRef string,
                llvm::SmallVectorImpl<llvm::StringRef> *matches = nullptr) const;
 
   /// Access the regular expression text.
   ///
   /// Returns the text that was used to compile the current regular
   /// expression.
   ///
   /// \return
   ///     The NULL terminated C string that was used to compile the
   ///     current regular expression
   llvm::StringRef GetText() const;
 
   /// Test if valid.
   ///
   /// Test if this object contains a valid regular expression.
   ///
   /// \return \b true if the regular expression compiled and is ready for
   ///     execution, \b false otherwise.
   bool IsValid() const;
 
   /// Return an error if the regular expression failed to compile.
   llvm::Error GetError() const;
 
 private:
   /// A copy of the original regular expression text.
   std::string m_regex_text;
   /// The compiled regular expression.
   mutable llvm::Regex m_regex;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_RegularExpression_h_

lldb/source/Breakpoint/BreakpointResolverName.cpp

 BreakpointResolverName::BreakpointResolverName(
     Breakpoint *bkpt, const char *name_cstr, FunctionNameType name_type_mask,
     LanguageType language, Breakpoint::MatchType type, lldb::addr_t offset,
     bool skip_prologue)
     : BreakpointResolver(bkpt, BreakpointResolver::NameResolver, offset),
       m_class_name(), m_regex(), m_match_type(type), m_language(language),
       m_skip_prologue(skip_prologue) {
   if (m_match_type == Breakpoint::Regexp) {
-    if (!m_regex.Compile(llvm::StringRef::withNullAsEmpty(name_cstr))) {
+    m_regex = RegularExpression(llvm::StringRef::withNullAsEmpty(name_cstr));
+    if (!m_regex.IsValid()) {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
 
       if (log)
         log->Warning("function name regexp: \"%s\" did not compile.",
                      name_cstr);
     }
   } else {
     AddNameLookup(ConstString(name_cstr), name_type_mask);

lldb/source/Commands/CommandCompletions.cpp

if (!m_request.GetCursorArgumentPrefix().empty()) {
     regex_str.append(".");
   }
   std::string::iterator pos =
       find_if(regex_str.begin() + 1, regex_str.end(), regex_chars);
   while (pos < regex_str.end()) {
     pos = regex_str.insert(pos, '\\');
     pos = find_if(pos + 2, regex_str.end(), regex_chars);
   }
-  m_regex.Compile(regex_str);
+  m_regex = RegularExpression(regex_str);
 }
 
 lldb::SearchDepth CommandCompletions::SymbolCompleter::GetDepth() {
   return lldb::eSearchDepthModule;
 }
 
 Searcher::CallbackReturn CommandCompletions::SymbolCompleter::SearchCallback(
     SearchFilter &filter, SymbolContext &context, Address *addr,

lldb/source/Commands/CommandObjectFrame.cpp

if (variable_list) {
         VariableList regex_var_list;
 
         // If we have any args to the variable command, we will make variable
         // objects from them...
         for (auto &entry : command) {
           if (m_option_variable.use_regex) {
             const size_t regex_start_index = regex_var_list.GetSize();
             llvm::StringRef name_str = entry.ref;
             RegularExpression regex(name_str);
-            if (regex.Compile(name_str)) {
+            if (regex.IsValid()) {
               size_t num_matches = 0;
               const size_t num_new_regex_vars =
                   variable_list->AppendVariablesIfUnique(regex, regex_var_list,
                                                          num_matches);
               if (num_new_regex_vars > 0) {
                 for (size_t regex_idx = regex_start_index,
                             end_index = regex_var_list.GetSize();
                      regex_idx < end_index; ++regex_idx) {

lldb/source/Commands/CommandObjectType.cpp

for (auto &arg_entry : command.entries()) {
       if (arg_entry.ref.empty()) {
         result.AppendError("empty typenames not allowed");
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
 
       ConstString typeCS(arg_entry.ref);
       if (m_command_options.m_regex) {
-        RegularExpressionSP typeRX(new RegularExpression());
-        if (!typeRX->Compile(arg_entry.ref)) {
+        RegularExpressionSP typeRX(new RegularExpression(arg_entry.ref));
+        if (!typeRX->IsValid()) {
           result.AppendError(
               "regex format error (maybe this is not really a regex?)");
           result.SetStatus(eReturnStatusFailed);
           return false;
         }
         category_sp->GetRegexTypeSummariesContainer()->Delete(typeCS);
         category_sp->GetRegexTypeFormatsContainer()->Add(typeRX, entry);
       } else
protected:
 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     const size_t argc = command.GetArgumentCount();
 
     std::unique_ptr<RegularExpression> category_regex;
     std::unique_ptr<RegularExpression> formatter_regex;
 
     if (m_options.m_category_regex.OptionWasSet()) {
-      category_regex.reset(new RegularExpression());
-      if (!category_regex->Compile(
-              m_options.m_category_regex.GetCurrentValueAsRef())) {
+      category_regex.reset(new RegularExpression(
+          m_options.m_category_regex.GetCurrentValueAsRef()));
+      if (!category_regex->IsValid()) {
         result.AppendErrorWithFormat(
             "syntax error in category regular expression '%s'",
             m_options.m_category_regex.GetCurrentValueAsRef().str().c_str());
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     }
 
     if (argc == 1) {
       const char *arg = command.GetArgumentAtIndex(0);
-      formatter_regex.reset(new RegularExpression());
-      if (!formatter_regex->Compile(llvm::StringRef::withNullAsEmpty(arg))) {
+      formatter_regex.reset(
+          new RegularExpression(llvm::StringRef::withNullAsEmpty(arg)));
+      if (!formatter_regex->IsValid()) {
         result.AppendErrorWithFormat("syntax error in regular expression '%s'",
                                      arg);
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     }
 
     bool any_printed = false;
DataVisualization::Categories::GetCategory(ConstString(category_name.c_str()),
                                              category);
 
   if (type == eRegularSummary) {
     if (FixArrayTypeNameWithRegex(type_name))
       type = eRegexSummary;
   }
 
   if (type == eRegexSummary) {
-    RegularExpressionSP typeRX(new RegularExpression());
-    if (!typeRX->Compile(type_name.GetStringRef())) {
+    RegularExpressionSP typeRX(new RegularExpression(type_name.GetStringRef()));
+    if (!typeRX->IsValid()) {
       if (error)
         error->SetErrorString(
             "regex format error (maybe this is not really a regex?)");
       return false;
     }
 
     category->GetRegexTypeSummariesContainer()->Delete(type_name);
     category->GetRegexTypeSummariesContainer()->Add(typeRX, entry);

 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     const size_t argc = command.GetArgumentCount();
 
     std::unique_ptr<RegularExpression> regex;
 
     if (argc == 1) {
-      regex.reset(new RegularExpression());
       const char *arg = command.GetArgumentAtIndex(0);
-      if (!regex->Compile(llvm::StringRef::withNullAsEmpty(arg))) {
+      regex.reset(new RegularExpression(llvm::StringRef::withNullAsEmpty(arg)));
+      if (!regex->IsValid()) {
         result.AppendErrorWithFormat(
             "syntax error in category regular expression '%s'", arg);
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     } else if (argc != 0) {
       result.AppendErrorWithFormat("%s takes 0 or one arg.\n",
                                    m_cmd_name.c_str());
if (category->AnyMatches(type_name, eFormatCategoryItemFilter |
     if (error)
       error->SetErrorStringWithFormat("cannot add synthetic for type %s when "
                                       "filter is defined in same category!",
                                       type_name.AsCString());
     return false;
   }
 
   if (type == eRegexSynth) {
-    RegularExpressionSP typeRX(new RegularExpression());
-    if (!typeRX->Compile(type_name.GetStringRef())) {
+    RegularExpressionSP typeRX(new RegularExpression(type_name.GetStringRef()));
+    if (!typeRX->IsValid()) {
       if (error)
         error->SetErrorString(
             "regex format error (maybe this is not really a regex?)");
       return false;
     }
 
     category->GetRegexTypeSyntheticsContainer()->Delete(type_name);
     category->GetRegexTypeSyntheticsContainer()->Add(typeRX, entry);
if (category->AnyMatches(type_name, eFormatCategoryItemSynth |
         error->SetErrorStringWithFormat("cannot add filter for type %s when "
                                         "synthetic is defined in same "
                                         "category!",
                                         type_name.AsCString());
       return false;
     }
 
     if (type == eRegexFilter) {
-      RegularExpressionSP typeRX(new RegularExpression());
-      if (!typeRX->Compile(type_name.GetStringRef())) {
+      RegularExpressionSP typeRX(new RegularExpression(type_name.GetStringRef()));
+      if (!typeRX->IsValid()) {
         if (error)
           error->SetErrorString(
               "regex format error (maybe this is not really a regex?)");
         return false;
       }
 
       category->GetRegexTypeFiltersContainer()->Delete(type_name);
       category->GetRegexTypeFiltersContainer()->Add(typeRX, entry);

lldb/source/Core/AddressResolverName.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 AddressResolverName::AddressResolverName(const char *func_name,
                                          AddressResolver::MatchType type)
     : AddressResolver(), m_func_name(func_name), m_class_name(nullptr),
       m_regex(), m_match_type(type) {
   if (m_match_type == AddressResolver::Regexp) {
-    if (!m_regex.Compile(m_func_name.GetStringRef())) {
+    m_regex = RegularExpression(m_func_name.GetStringRef());
+    if (!m_regex.IsValid()) {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
 
       if (log)
         log->Warning("function name regexp: \"%s\" did not compile.",
                      m_func_name.AsCString());
     }
   }
 }

lldb/source/Interpreter/CommandObjectRegexCommand.cpp

result.GetOutputStream() << "Command contents '" << command
                              << m_cmd_name << "' regex ";
   return false;
 }
 
 bool CommandObjectRegexCommand::AddRegexCommand(const char *re_cstr,
                                                 const char *command_cstr) {
   m_entries.resize(m_entries.size() + 1);
   // Only add the regular expression if it compiles
-  if (m_entries.back().regex.Compile(
-          llvm::StringRef::withNullAsEmpty(re_cstr))) {
+  m_entries.back().regex =
+      RegularExpression(llvm::StringRef::withNullAsEmpty(re_cstr));
+  if (m_entries.back().regex.IsValid()) {
     m_entries.back().command.assign(command_cstr);
     return true;
   }
   // The regex didn't compile...
   m_entries.pop_back();
   return false;
 }
 

lldb/source/Interpreter/OptionValueRegex.cpp

Status OptionValueRegex::SetValueFromString(llvm::StringRef value,
 
   case eVarSetOperationClear:
     Clear();
     NotifyValueChanged();
     break;
 
   case eVarSetOperationReplace:
   case eVarSetOperationAssign:
-    if (m_regex.Compile(value)) {
+    m_regex = RegularExpression(value);
+    if (m_regex.IsValid()) {
       m_value_was_set = true;
       NotifyValueChanged();
     } else if (llvm::Error err = m_regex.GetError()) {
       error.SetErrorString(llvm::toString(std::move(err)));
     } else {
       error.SetErrorString("regex error");
     }
     break;
   }
   return error;
 }
 
 lldb::OptionValueSP OptionValueRegex::DeepCopy() const {
   return OptionValueSP(new OptionValueRegex(m_regex.GetText().str().c_str()));
 }

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     std::unique_ptr<RegularExpression> regex_up;
     switch (command.GetArgumentCount()) {
     case 0:
       break;
     case 1: {
-      regex_up.reset(new RegularExpression());
-      if (!regex_up->Compile(llvm::StringRef::withNullAsEmpty(
-              command.GetArgumentAtIndex(0)))) {
+      regex_up.reset(new RegularExpression(
+          llvm::StringRef::withNullAsEmpty(command.GetArgumentAtIndex(0))));
+      if (!regex_up->IsValid()) {
         result.AppendError(
             "invalid argument - please provide a valid regular expression");
         result.SetStatus(lldb::eReturnStatusFailed);
         return false;
       }
       break;
     }
     default: {

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

 
 bool ParseCoordinate(llvm::StringRef coord_s, RSCoordinate &coord) {
   // takes an argument of the form 'num[,num][,num]'. Where 'coord_s' is a
   // comma separated 1,2 or 3-dimensional coordinate with the whitespace
   // trimmed. Missing coordinates are defaulted to zero. If parsing of any
   // elements fails the contents of &coord are undefined and `false` is
   // returned, `true` otherwise
 
   RegularExpression regex;
   llvm::SmallVector<llvm::StringRef, 4> matches;
 
   bool matched = false;
-  if (regex.Compile(llvm::StringRef("^([0-9]+),([0-9]+),([0-9]+)$")) &&
-      regex.Execute(coord_s, &matches))
+  if (!matched) {
+    regex = RegularExpression(llvm::StringRef("^([0-9]+),([0-9]+),([0-9]+)$"));
+    if (regex.Execute(coord_s, &matches))
       matched = true;
-  else if (regex.Compile(llvm::StringRef("^([0-9]+),([0-9]+)$")) &&
-           regex.Execute(coord_s, &matches))
+  }
+  if (!matched) {
+    regex = RegularExpression(llvm::StringRef("^([0-9]+),([0-9]+)$"));
+    if (regex.Execute(coord_s, &matches))
       matched = true;
-  else if (regex.Compile(llvm::StringRef("^([0-9]+)$")) &&
-           regex.Execute(coord_s, &matches))
+  }
+  if (!matched) {
+    regex = RegularExpression(llvm::StringRef("^([0-9]+)$"));
+    if (regex.Execute(coord_s, &matches))
       matched = true;
+  }
 
   if (!matched)
     return false;
 
   auto get_index = [&](size_t idx, uint32_t &i) -> bool {
     std::string group;
     errno = 0;
     if (idx + 1 < matches.size()) {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

if (bytes_read > 0) {
           RegularExpression response_regex;
 
           if (m_supports_qEcho == eLazyBoolYes) {
             echo_packet_len = ::snprintf(echo_packet, sizeof(echo_packet),
                                          "qEcho:%u", ++m_echo_number);
             std::string regex_str = "^";
             regex_str += echo_packet;
             regex_str += "$";
-            response_regex.Compile(regex_str);
+            response_regex = RegularExpression(regex_str);
           } else {
             echo_packet_len =
                 ::snprintf(echo_packet, sizeof(echo_packet), "qC");
-            response_regex.Compile(llvm::StringRef("^QC[0-9A-Fa-f]+$"));
+            response_regex =
+                RegularExpression(llvm::StringRef("^QC[0-9A-Fa-f]+$"));
           }
 
           PacketResult echo_packet_result =
               SendPacketNoLock(llvm::StringRef(echo_packet, echo_packet_len));
           if (echo_packet_result == PacketResult::Success) {
             const uint32_t max_retries = 3;
             uint32_t successful_responses = 0;
             for (uint32_t i = 0; i < max_retries; ++i) {

lldb/source/Target/ThreadPlanStepInRange.cpp

if (!m_sub_plan_sp) {
     m_sub_plan_sp->SetPrivate(true);
     return false;
   }
 }
 
 void ThreadPlanStepInRange::SetAvoidRegexp(const char *name) {
   auto name_ref = llvm::StringRef::withNullAsEmpty(name);
   if (!m_avoid_regexp_up)
     m_avoid_regexp_up.reset(new RegularExpression(name_ref));
-
-  m_avoid_regexp_up->Compile(name_ref);
+  else
+    *m_avoid_regexp_up = RegularExpression(name_ref);
 }
 
 void ThreadPlanStepInRange::SetDefaultFlagValue(uint32_t new_value) {
   // TODO: Should we test this for sanity?
   ThreadPlanStepInRange::s_default_flag_values = new_value;
 }
 
 bool ThreadPlanStepInRange::FrameMatchesAvoidCriteria() {
   StackFrame *frame = GetThread().GetStackFrameAtIndex(0).get();

lldb/source/Utility/RegularExpression.cpp

 //===-- RegularExpression.cpp -----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/RegularExpression.h"
 
 #include <string>
 
 using namespace lldb_private;
 
-RegularExpression::RegularExpression(llvm::StringRef str) { Compile(str); }
-
-RegularExpression::RegularExpression(const RegularExpression &rhs)
-    : RegularExpression() {
-  Compile(rhs.GetText());
-}
-
-bool RegularExpression::Compile(llvm::StringRef str) {
+RegularExpression::RegularExpression(llvm::StringRef str) {
   m_regex_text = str;
+
+  // m_regex does not reference str anymore after it is constructed.
   m_regex = llvm::Regex(str);
-  return IsValid();
 }
 
+RegularExpression::RegularExpression(const RegularExpression &rhs)
+    : RegularExpression(rhs.GetText()) {}
+
 bool RegularExpression::Execute(
     llvm::StringRef str,
     llvm::SmallVectorImpl<llvm::StringRef> *matches) const {
+  if (!IsValid())
+    return false;
   return m_regex.match(str, matches);
 }
 
 bool RegularExpression::IsValid() const {
-  std::string discarded;
-  return m_regex.isValid(discarded);
+  return m_regex.isValid();
 }
 
 llvm::StringRef RegularExpression::GetText() const { return m_regex_text; }
 
 llvm::Error RegularExpression::GetError() const {
   std::string error;
   if (!m_regex.isValid(error))
     return llvm::make_error<llvm::StringError>(llvm::inconvertibleErrorCode(),
                                                error);
   return llvm::Error::success();
 }

llvm/include/llvm/Support/Regex.h

enum {
       /// By default, the POSIX extended regular expression (ERE) syntax is
       /// assumed. Pass this flag to turn on basic regular expressions (BRE)
       /// instead.
       BasicRegex=4
     };
 
     Regex();
     /// Compiles the given regular expression \p Regex.
+    ///
+    /// \param Regex - referenced string lifetime is no longer needed after this
+    /// constructor does finish.  Only its compiled form is kept stored.
     Regex(StringRef Regex, unsigned Flags = NoFlags);
     Regex(const Regex &) = delete;
     Regex &operator=(Regex regex) {
       std::swap(preg, regex.preg);
       std::swap(error, regex.error);
       return *this;
     }
     Regex(Regex &&regex);
     ~Regex();
 
     /// isValid - returns the error encountered during regex compilation, or
     /// matching, if any.
     bool isValid(std::string &Error) const;
+    bool isValid() const { return !error; }
 
     /// getNumMatches - In a valid regex, return the number of parenthesized
     /// matches it contains.  The number filled in by match will include this
     /// many entries plus one for the whole regex (as element 0).
     unsigned getNumMatches() const;
 
     /// matches - Match the regex against a given \p String.
     ///