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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Haven't made it all the way through, but I think it would be helpful to apply some of the comments so far across all the changes here.



================
Comment at: lib/Passes/PassBuilder.cpp:149-150
 
-static Regex DefaultAliasRegex("^(default|lto-pre-link|lto)<(O[0123sz])>$");
+static Regex DefaultAliasRegex = ExitOnError("invalid regex literal")(
+    Regex::compile("^(default|lto-pre-link|lto)<(O[0123sz])>$"));
 
----------------
Is it important to use ExitOnError here? I'm thinkign something like compileKnownValid might be more appropriate.


================
Comment at: lib/Support/SpecialCaseList.cpp:127
     // Check that the regexp is valid.
-    Regex CheckRE(Regexp);
-    std::string REError;
-    if (!CheckRE.isValid(REError)) {
+    Regex CheckRE;
+    if (auto RegexpOrError = Regex::compile(Regexp)) {
----------------
This isn't used any more. I think you can simplify the code a lot by just getting the error, and inverting the condition below.


================
Comment at: lib/Support/SpecialCaseList.cpp:157
+      Entries[I->getKey()][II->getKey()].RegEx = llvm::make_unique<Regex>(
+          ExitOnError("combined validated regexes were somehow invalid")(
+              Regex::compile(II->getValue())));
----------------
This also seems like it could be compileKnownValid, as you have validated them above.


================
Comment at: lib/Target/AArch64/Utils/AArch64BaseInfo.cpp:91-92
   SmallVector<StringRef, 5> Ops;
-  if (!GenericRegPattern.match(UpperName, &Ops))
+  // Note that if GenericRegPattern failed to compile, this does not handle
+  // that error and will crash with asserts enabled.
+  if (GenericRegPattern && !GenericRegPattern->match(UpperName, &Ops))
----------------
compileKnownValid again?


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:608-611
+        if (auto RegexOrError = llvm::Regex::compile(Regex)) {
+          NameFilterer->push_back(llvm::make_unique<NameRegexCoverageFilter>(
+              std::move(*RegexOrError)));
+        } else {
----------------
I still would prefer early error exiting here so:

  auto RegexOrError = ...
  if (!RegexORError) {
    ...
    return ...;
  }
  ...


================
Comment at: tools/llvm-pdbdump/LinePrinter.h:46-52
+      if (auto RegexOrError = Regex::compile(*Begin)) {
+        List.emplace_back(std::move(*RegexOrError));
+      } else if (Err) {
+        *Err = joinErrors(std::move(*Err), RegexOrError.takeError());
+      } else {
+        consumeError(RegexOrError.takeError());
+      }
----------------
Use continue on error to reduce non-error indentation?


https://reviews.llvm.org/D27463