[PATCH] D27463: Change llvm::Regex to expose a fallible constructor.

[David L. Jones via Phabricator via llvm-commits]

dlj added a comment.

In https://reviews.llvm.org/D27463#615399, @vsk wrote:

> If the goal is to make sure regex errors are checked, we could avoid a substantial amount of API churn by asserting 'no error' in every method in Regex.


That's the end goal I'm aiming for in the current change, but by a different route: for regexes compiled from static char[] constants, I'm just letting the Expected<Regex> propagate the error. It's a bit subtle, but I'll add comments. With asserts disabled, a failed compilation for a constant will simply look like it didn't match.

For "constants," I think this is the right thing. It limits API churn, since things that should never fail don't need to propagate an error, they just assert. For non-constants, I'm intending to propagate errors in whatever way is consistent.

(Perhaps unsurprisingly, there are, in fact, some broken regex literals in the code base.)

> If you also want to make match() const, we'll need to get rid of the isValid method and the error field. That probably means adding an optional out-param to the Regex constructor for error descriptions.

Right, I was planning to eliminate the error field in a separate change. The Expected<> wrapper obviates the need for an out-parameter to the constructor, though. Anywhere an out-param would be used can use the Expected<> instead.


https://reviews.llvm.org/D27463