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

[Mark de Wever via Phabricator via libcxx-commits]

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


================
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());
----------------
Mordante wrote:
> 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.
> > 
> > WDYT?
> 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
> 
> Agreed?
@ldionne Do you agree with the proposed approach? 


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

https://reviews.llvm.org/D63059