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

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

thopre 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;
----------------
nlguillemot wrote:
> thopre wrote:
> > nlguillemot wrote:
> > > 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?
> > Indeed, sounds like a good idea. Yes I can commit the patch for you once it is approved.
> > 
> > Best regards.
> Thanks for helping me with the commit. Is it ok if I switch around the reviewers to make you the reviewer? There was no particular reason for the choice of the initial reviewer list other than the recent activity in the file, so I think your review feedback is probably enough.
That's usually the right way to go about it since someone who did a change in an area is likely to be interested in what's going on in that area in the future. You can add me to the reviewer list yes, but no need to remove anyone I think.


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

https://reviews.llvm.org/D67241