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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

In D68054#1686276 <https://reviews.llvm.org/D68054#1686276>, @nlguillemot wrote:

> 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?


I'm quite new to LLVM community but my understanding is that the general approach is to not worry about external dependencies. On the other hand the benefit is small, doing a check again in match and sub is cheap. We can still have those convenience function to avoid using a temporary variable, so I'd tend to agree with you and go for option B. Update the patch to keep the checks for errors in sub and match and I'll approve it.


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

https://reviews.llvm.org/D68054