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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/regex:2116
+void
+__r_anchor_multiline<_CharT>::__exec(__state& __s) const
+{
----------------
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`.


================
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());
----------------
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?


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

https://reviews.llvm.org/D63059