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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment 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());
----------------
ldionne wrote:
> Mordante wrote:
> > 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? 
> Sorry this got dropped. Yes, we agree on the approach.
> 
> Regarding
> 
> > 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. [...]
> 
> My reasoning is that since it's a class template that isn't explicitly instantiated in and exported from the dylib, any code that references `__l_anchor` already has a weak definition for the vtable. It needs to, because that vtable isn't defined anywhere else the linker can tell upfront.
> 
> So, just removing that class from the headers isn't going to break anything, because
> 1. already-compiled code already has the definitions it needs, and
> 2. we're not changing an existing definition to do something incompatible (since we're introducing a new one).
> 
> ABIs are tricky, so I hope I'm not missing something. But if my reasoning holds, your approach LGTM. Let's go with that.
Thanks for the explanation. 


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

https://reviews.llvm.org/D63059