[Lldb-commits] [PATCH] D66392: 1/2: D66174 `RegularExpression` cleanup
Jan Kratochvil via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Aug 19 07:34:58 PDT 2019
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:
Is it complicated to get the `llvm/` extension approved? I can quickly drop any parts of this patch not considered as feasible.
CHANGES SINCE LAST ACTION
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 16133 bytes
Desc: not available
More information about the lldb-commits