[PATCH] D27465: Use the fallible llvm::Regex constructor in Clang.

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 6 17:21:57 PST 2016


vsk added inline comments.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:156
+  if (auto RegexOrError = llvm::Regex::compile(RegexText)) {
+    return std::move(*RegexOrError);
+  }
----------------
If there *is* an error here, then implicitly converting the Expected instance to a bool doesn't mark the error as checked. I.e in asserts mode, you'll abort before you make it out of this if body. If that's really what you want, I think an assert would be a clearer / more lightweight way of doing it. This applies elsewhere in your patch: I think it makes sense to use Expected as a kind of exception mechanism, and not as a heavyweight assert.


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:234
+  static llvm::Expected<llvm::Regex> Matchers[] = {
+      llvm::Regex::compile("^.*$"),
+      llvm::Regex::compile("^[a-z][a-z0-9_]*$"),
----------------
Could you explain the rationale behind this change? Since we can verify that the regexes are valid at compile-time, why is there a need to wrap them with Expected?


https://reviews.llvm.org/D27465





More information about the llvm-commits mailing list