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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 215890.
jankratochvil retitled this revision from "1/2: D66174 `RegularExpression` follow-up/cleanup" to "1/2: D66174 `RegularExpression` cleanup".
jankratochvil added a comment.

In D66392#1634575 <https://reviews.llvm.org/D66392#1634575>, @JDevlieghere wrote:

> - Thanks for removing the stale inheritance! (LGTM)


Checked it in now as rL369235 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D66398> correctly). (LGTM)

Yes, you are right the operators were not used.  So I have also moved them now to D66398 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66392

Files:
  lldb/include/lldb/Interpreter/OptionValueRegex.h
  lldb/include/lldb/Utility/RegularExpression.h
  lldb/source/Breakpoint/BreakpointResolverName.cpp
  lldb/source/Commands/CommandCompletions.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Core/AddressResolverName.cpp
  lldb/source/Interpreter/CommandObjectRegexCommand.cpp
  lldb/source/Interpreter/OptionValueRegex.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Target/ThreadPlanStepInRange.cpp
  lldb/source/Utility/RegularExpression.cpp
  llvm/include/llvm/Support/Regex.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66392.215890.patch
Type: text/x-patch
Size: 16133 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190819/55f33f19/attachment-0001.bin>