[libcxx-commits] [PATCH] D63059: Implements multiline regex support

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sat Mar 21 09:37:31 PDT 2020

Mordante marked 4 inline comments as done.
Mordante added inline comments.

Comment at: libcxx/include/regex:2116
+__r_anchor_multiline<_CharT>::__exec(__state& __s) const
ldionne wrote:
> To avoid code duplication, we could create a helper function that takes a `bool __multiline` parameter. Then, `__r_anchor_multiline<_CharT>::__exec` would call that helper function with `__multiline` as an argument, and `__r_anchor<_CharT>::__exec` would call the helper function with `false`.
That's indeed possible. I can create an updated patch with that approach.
(Before updating I want to look what approach we take regarding your comments on `__l_anchor`.)

Comment at: libcxx/include/regex:4840
-    __end_->first() = new __l_anchor<_CharT>(__end_->first());
+    __end_->first() = new __l_anchor_multiline<_CharT>(__use_multiline(), __end_->first());
     __end_ = static_cast<__owns_one_state<_CharT>*>(__end_->first());
ldionne wrote:
> Do we ever create a `__l_anchor` anymore? If not, then we could in theory delete that code. Actually, now that I think of it, I believe we could simply add the functionality to `__l_anchor` itself, but rename the class to something else. The idea is:
> - No new code needs to use `__l_anchor`, it can all start using the new class
> - Existing code using `__l_anchor` already has a vtable for `__l_anchor`, and that contains the old implementation of `execute` they need to keep using to avoid and ABI break.
> I believe the ABI break only happens if we change `__l_anchor::execute` without changing the name of the class, since that's when two libraries compiled at different times (and hence with different versions of `__l_anchor`) can disagree on the layout of `__l_anchor`. If we change the name of the class, I think it should all work out.
No after this change we'll never create a `__l_anchor`.  I expected removing the `__l_anchor` class from the header would be an ABI break. But I think you're right since this is a templated class. So that means I:
* Let the `__l_anchor_multiline` directly inherit from `__owns_one_state<_CharT>`
* Remove the `__l_anchor` class
* Do the same for the 'right' part and thus don't need the helper function as suggested before




More information about the libcxx-commits mailing list