[Lldb-commits] [PATCH] D66392: 1/2: D66174 `RegularExpression` cleanup

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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...



================
Comment at: lldb/include/lldb/Interpreter/OptionValueRegex.h:53-55
-      m_regex.Compile(llvm::StringRef(value));
+      m_regex = RegularExpression(llvm::StringRef(value));
     else
       m_regex = RegularExpression();
----------------
The apparent inconsistency here is a pretty good argument against the `Compile` method imo.


================
Comment at: lldb/include/lldb/Utility/RegularExpression.h:34
+  /// \param[in] string
+  ///     A NULL terminated C string that represents the regular
+  ///     expression to compile.
----------------
Null terminated? I hope not..


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:536-537
             llvm::StringRef name_str = entry.ref;
             RegularExpression regex(name_str);
-            if (regex.Compile(name_str)) {
               size_t num_matches = 0;
----------------
lol


================
Comment at: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp:445
 
   RegularExpression regex;
   llvm::SmallVector<llvm::StringRef, 4> matches;
----------------
Here I'd just declare a fresh variable in each of the blocks instead of continuously recycling this one.


================
Comment at: lldb/source/Target/ThreadPlanStepInRange.cpp:317-319
     m_avoid_regexp_up.reset(new RegularExpression(name_ref));
-
-  m_avoid_regexp_up->Compile(name_ref);
+  else
+    *m_avoid_regexp_up = RegularExpression(name_ref);
----------------
maybe swap the then and else blocks around to reduce the amount of negation


================
Comment at: lldb/source/Utility/RegularExpression.cpp:16-19
   m_regex_text = str;
+
+  // m_regex does not reference str anymore after it is constructed.
   m_regex = llvm::Regex(str);
----------------
Initialize this stuff in the member initializer list?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66392/new/

https://reviews.llvm.org/D66392