[PATCH] D68054: Regex: Add static convenience functions for "match" and "sub"

[Nicolas Guillemot via Phabricator via llvm-commits]

nlguillemot added a comment.

In D68054#1685676 <https://reviews.llvm.org/D68054#1685676>, @thopre wrote:

> The following clang unit tests fail with your patch:
>
> Format/./FormatTests/FormatTest.FunctionAnnotations
>  Format/./FormatTests/FormatTest.UnderstandsFunctionRefQualification
>
> Can you have a look?


Oops, I didn't even think of checking if clang works. I started taking a look at it, and here's what I think we should do:

1. We should do another Regex const correctness patch for clang's uses of Regex.
2. We need to decide how to deal with the fact that clang depends on match()/sub()'s behavior of lazily reporting compile errors:

  Option A) Make a patch for llvm and clang to put before this one that adds explicit `isValid()` checks to all uses of match()/sub() with "untrusted" inputs.

  Option B) Keep the previous behavior and return false instead of asserting inside `match()`, for the sake of backwards compatibility.

If we do option A, we don't compromise on one of the original goals of this patch, and we make the API of match()/sub() more strict.
If we do option B, we have less chance of potentially breaking code in other projects in the LLVM ecosystem.

I think step (1) is a win-win, but I'm not sure about step (2). I'm tempted to conservatively go with the backwards compatible option B, but if we can judge that the potential impact of changing the API with option A is acceptable, we could go ahead and take that risk anyways.

Thoughts?


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

https://reviews.llvm.org/D68054