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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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:
> 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.


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

https://reviews.llvm.org/D63059