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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D63059#1920509 <https://reviews.llvm.org/D63059#1920509>, @ldionne wrote:

> In D63059#1920398 <https://reviews.llvm.org/D63059#1920398>, @Mordante wrote:
>
> > Thanks for the confirmation it's an ABI break. I like your suggestion to use a derived class, so I'll try that solution first.
>
>
> I'm not 100% sure that solution addresses the ABI break problem, though. I'd have to think about it harder and run it through at least another person to be confident.


I'll update the diff with this approach in a sec.

> Can you comment on that:
> 
>> Do we have a way of quantifying how bad it would be? What regexes would be impacted? Is it regexes with ^ or $ used as anchors, passed through ABI boundaries?

Yes it requires a regex with a `^` or `$` anchor. It should only break if created with the old version (no bool in the struct) and executed by the new version (with a bool in the struct)

> If that's an ABI break we can afford to make, it would be great cause it would avoid increasing the code complexity.

Googling/looking in GitHub is hard since it ignores special characters. I searched on https://codesearch.debian.net
and found 318 hits looking for `std::regex("^` 205 were in the llvm projects so about 100 in other projects.
Looking at `std::regex\(".*\$` found 309 matches with 177 in the llvm projects. This regex hit some false positives.

So at least in Debian it is not used that often. So we have 3 options:

1. Use the version with inheritance if ABI safe. Pro no `#ifdef`s, Con adds some 'dead code' for ABI compatibility
2. Replace `#if _LIBCPP_STD_VER > 14` of the current patch with an ABI flag. Pro ABI compatible, Con adds a lot if `#ifdefs`
3. Break the ABI, Pro cleanest code, Con an ABI breakage

If 1 is ABI safe I would prefer that option, else I would slightly prefer to avoid an ABI breakage.

What's your opinion?


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

https://reviews.llvm.org/D63059