[PATCH] D67241: Regex: Make "match" and "sub" const member functions

[Nicolas Guillemot via Phabricator via llvm-commits]

nlguillemot marked an inline comment as done.
nlguillemot added inline comments.


================
Comment at: lib/Support/Regex.cpp:88-90
+  // Check if the regex itself didn't successfully compile.
+  if (Error ? !isValid(*Error) : !isValid())
     return false;
----------------
thopre wrote:
> nlguillemot wrote:
> > thopre wrote:
> > > I think I'd prefer the API to enforce the user to check compile error when creating the regex and just have an assert here. match should only report errors in matching IMHO.
> > I completely agree in principle, though right now that would cause some API breakage.
> > For example, the following usage in `lib/Transforms/Utils/SymbolRewriter.cpp`:
> > 
> > ```
> >      std::string Name = Regex(Pattern).sub(Transform, C.getName(), &Error);
> >      if (!Error.empty())
> >        report_fatal_error("unable to transforn " + C.getName() + " in " +
> >                           M.getModuleIdentifier() + ": " + Error);
> > ```
> > 
> > I guess we could make the change anyways and update all the call sites. Maybe good for a follow-up patch?
> Yes, doing it in a separate would be nice. Ideally update call sites in a first patch and then apply this patch on top with those asserts I suggested.
I think it would be nice if there was an alternative way to cleanly implement the "Create a Regex and immediately throw it away" idiom as used in the example in `SymbolRewriter.cpp` above.

For example, Python's API can be used both ways:
```
import re
# "one-shot" regex
re.match(some_pattern, some_string)
# pre-compiled regex
compiled = re.compile(some_pattern)
compiled.match(some_string)
```
Maybe we should have something similar, where the whole process of creating the Regex object and throwing it away is wrapped in one function call. It could maybe be implemented by static member functions where the first parameter is the regex pattern. That would also make the refactoring more easy for cases like the above.

By the way, I don't have commit access to LLVM yet. If the patch looks good to you so far, could you commit it on my behalf?


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

https://reviews.llvm.org/D67241