Use the fallible llvm::Regex constructor in Clang.

[dlj]

Summary

This change updates Clang to check for an llvm::Expected<llvm::Regex>, instead
of constructing an invalid Regex and checking .isValid().

Since most of the uses already (silently) ignore invalid llvm::Regex objects,
I've not tried too hard to propagate construction-time errors.

[vsk]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

156

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.

clang-tidy/readability/IdentifierNamingCheck.cpp

234

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?

[dlj]

• Fix long line.

[dlj]

• Fix long line.
• Fix some broken regexes.

[dlj]

Ping?

change-namespace/ChangeNamespace.cpp

 } // anonymous namespace
 
 ChangeNamespaceTool::ChangeNamespaceTool(
     llvm::StringRef OldNs, llvm::StringRef NewNs, llvm::StringRef FilePattern,
     std::map<std::string, tooling::Replacements> *FileToReplacements,
     llvm::StringRef FallbackStyle)
     : FallbackStyle(FallbackStyle), FileToReplacements(*FileToReplacements),
       OldNamespace(OldNs.ltrim(':')), NewNamespace(NewNs.ltrim(':')),
-      FilePattern(FilePattern), FilePatternRE(FilePattern) {
+      FilePattern(FilePattern) {
+  if (auto RegexOrError = llvm::Regex::compile(FilePattern)) {
+    FilePatternRE = std::move(*RegexOrError);
+  }
   FileToReplacements->clear();
   llvm::SmallVector<llvm::StringRef, 4> OldNsSplitted;
   llvm::SmallVector<llvm::StringRef, 4> NewNsSplitted;
   llvm::StringRef(OldNamespace).split(OldNsSplitted, "::");
   llvm::StringRef(NewNamespace).split(NewNsSplitted, "::");
   // Calculates `DiffOldNamespace` and `DiffNewNamespace`.
   while (!OldNsSplitted.empty() && !NewNsSplitted.empty() &&
          OldNsSplitted.front() == NewNsSplitted.front()) {

clang-tidy/ClangTidyDiagnosticConsumer.cpp

static llvm::Regex ConsumeGlob(StringRef &GlobList) {
   for (char C : Glob) {
     if (C == '*')
       RegexText.push_back('.');
     else if (MetaChars.find(C) != StringRef::npos)
       RegexText.push_back('\\');
     RegexText.push_back(C);
   }
   RegexText.push_back('$');
-  return llvm::Regex(RegexText);
+  if (auto RegexOrError = llvm::Regex::compile(RegexText)) {
+    return std::move(*RegexOrError);
+  }
+  return llvm::Regex();
 }
 
 GlobList::GlobList(StringRef Globs)
     : Positive(!ConsumeNegativeIndicator(Globs)), Regex(ConsumeGlob(Globs)),
       NextGlob(Globs.empty() ? nullptr : new GlobList(Globs)) {}
 
 bool GlobList::contains(StringRef S, bool Contains) {
   if (Regex.match(S))
LastErrorRelatesToUserCode = LastErrorRelatesToUserCode ||
                                getHeaderFilter()->match(FileName);
 
   unsigned LineNumber = Sources.getExpansionLineNumber(Location);
   LastErrorPassesLineFilter =
       LastErrorPassesLineFilter || passesLineFilter(FileName, LineNumber);
 }
 
 llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
-  if (!HeaderFilter)
-    HeaderFilter.reset(
-        new llvm::Regex(*Context.getOptions().HeaderFilterRegex));
+  if (!HeaderFilter) {
+    HeaderFilter.reset(new llvm::Regex);
+    if (auto RegexOrError =
+            llvm::Regex::compile(*Context.getOptions().HeaderFilterRegex)) {
+      *HeaderFilter = std::move(*RegexOrError);
+    }
+  }
   return HeaderFilter.get();
 }
 
 void ClangTidyDiagnosticConsumer::removeIncompatibleErrors(
     SmallVectorImpl<ClangTidyError> &Errors) const {
   // Each error is modelled as the set of intervals in which it applies
   // replacements. To detect overlapping replacements, we use a sweep line
   // algorithm over these sets of intervals.

clang-tidy/google/TodoCommentCheck.cpp

 namespace clang {
 namespace tidy {
 namespace google {
 namespace readability {
 
 class TodoCommentCheck::TodoCommentHandler : public CommentHandler {
 public:
   TodoCommentHandler(TodoCommentCheck &Check, llvm::Optional<std::string> User)
-      : Check(Check), User(User ? *User : "unknown"),
-        TodoMatch("^// *TODO *(\\(.*\\))?:?( )?(.*)$") {}
+      : Check(Check), User(User ? *User : "unknown") {
+    if (auto RegexOrError =
+            llvm::Regex::compile("^// *TODO *(\\(.*\\))?:?( )?(.*)$")) {
+      TodoMatch = std::move(*RegexOrError);
+    }
+  }
 
   bool HandleComment(Preprocessor &PP, SourceRange Range) override {
     StringRef Text =
         Lexer::getSourceText(CharSourceRange::getCharRange(Range),
                              PP.getSourceManager(), PP.getLangOpts());
 
     SmallVector<StringRef, 4> Matches;
     if (!TodoMatch.match(Text, &Matches))

clang-tidy/misc/ArgumentCommentCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0),
-      IdentRE("^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$") {}
+      StrictMode(Options.getLocalOrGlobal("StrictMode", 0) != 0) {
+  if (auto RegexOrError = llvm::Regex::compile(
+          "^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$")) {
+    IdentRE = std::move(*RegexOrError);
+  }
+}
 
 void ArgumentCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "StrictMode", StrictMode);
 }
 
 void ArgumentCommentCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       callExpr(unless(cxxOperatorCallExpr()),

clang-tidy/modernize/AvoidBindCheck.cpp

struct BindArgument {
   size_t PlaceHolderIndex = 0;
 };
 
 } // end namespace
 
 static SmallVector<BindArgument, 4>
 buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) {
   SmallVector<BindArgument, 4> BindArguments;
-  llvm::Regex MatchPlaceholder("^_([0-9]+)$");
+  auto MatchPlaceholder = llvm::ExitOnError("bad regex literal")(
+      llvm::Regex::compile("^_([0-9]+)$"));
 
   // Start at index 1 as first argument to bind is the function name.
   for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) {
     const Expr *E = C->getArg(I);
     BindArgument B;
     if (const auto *M = dyn_cast<MaterializeTemporaryExpr>(E)) {
       const auto *TE = M->GetTemporaryExpr();
       B.Kind = isa<CallExpr>(TE) ? BK_CallExpr : BK_Temporary;
     }
 
     B.Tokens = Lexer::getSourceText(
         CharSourceRange::getTokenRange(E->getLocStart(), E->getLocEnd()),
         *Result.SourceManager, Result.Context->getLangOpts());
 
     SmallVector<StringRef, 2> Matches;
-    if (B.Kind == BK_Other && MatchPlaceholder.match(B.Tokens, &Matches)) {
+    if (B.Kind == BK_Other &&
+        MatchPlaceholder.match(B.Tokens, &Matches)) {
       B.Kind = BK_Placeholder;
       B.PlaceHolderIndex = std::stoi(Matches[1]);
     }
     BindArguments.push_back(B);
   }
   return BindArguments;
 }
 

clang-tidy/readability/IdentifierNamingCheck.cpp

 void IdentifierNamingCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Compiler.getPreprocessor().addPPCallbacks(
       llvm::make_unique<IdentifierNamingCheckPPCallbacks>(
           &Compiler.getPreprocessor(), this));
 }
 
 static bool matchesStyle(StringRef Name,
                          IdentifierNamingCheck::NamingStyle Style) {
-  static llvm::Regex Matchers[] = {
-      llvm::Regex("^.*$"),
-      llvm::Regex("^[a-z][a-z0-9_]*$"),
-      llvm::Regex("^[a-z][a-zA-Z0-9]*$"),
-      llvm::Regex("^[A-Z][A-Z0-9_]*$"),
-      llvm::Regex("^[A-Z][a-zA-Z0-9]*$"),
-      llvm::Regex("^[A-Z]([a-z0-9]*(_[A-Z])?)*"),
-      llvm::Regex("^[a-z]([a-z0-9]*(_[A-Z])?)*"),
+  static llvm::Expected<llvm::Regex> Matchers[] = {
+      llvm::Regex::compile("^.*$"),
+      llvm::Regex::compile("^[a-z][a-z0-9_]*$"),
+      llvm::Regex::compile("^[a-z][a-zA-Z0-9]*$"),
+      llvm::Regex::compile("^[A-Z][A-Z0-9_]*$"),
+      llvm::Regex::compile("^[A-Z][a-zA-Z0-9]*$"),
+      llvm::Regex::compile("^[A-Z]([a-z0-9]*(_[A-Z])?)*"),
+      llvm::Regex::compile("^[a-z]([a-z0-9]*(_[A-Z])?)*"),
   };
 
   bool Matches = true;
   if (Name.startswith(Style.Prefix))
     Name = Name.drop_front(Style.Prefix.size());
   else
     Matches = false;
 
   if (Name.endswith(Style.Suffix))
     Name = Name.drop_back(Style.Suffix.size());
   else
     Matches = false;
 
-  if (!Matchers[static_cast<size_t>(Style.Case)].match(Name))
+  auto &M = Matchers[static_cast<size_t>(Style.Case)];
+  if (!(M && M->match(Name)))
     Matches = false;
 
   return Matches;
 }
 
 static std::string fixupWithCase(StringRef Name,
                                  IdentifierNamingCheck::CaseType Case) {
-  static llvm::Regex Splitter(
+  static auto Splitter = llvm::Regex::compile(
       "([a-z0-9A-Z]*)(_+)|([A-Z]?[a-z0-9]+)([A-Z]|$)|([A-Z]+)([A-Z]|$)");
 
   SmallVector<StringRef, 8> Substrs;
   Name.split(Substrs, "_", -1, false);
 
   SmallVector<StringRef, 8> Words;
   for (auto Substr : Substrs) {
     while (!Substr.empty()) {
       SmallVector<StringRef, 8> Groups;
-      if (!Splitter.match(Substr, &Groups))
+      if (!(Splitter && Splitter->match(Substr, &Groups)))
         break;
 
       if (Groups[2].size() > 0) {
         Words.push_back(Groups[1]);
         Substr = Substr.substr(Groups[0].size());
       } else if (Groups[3].size() > 0) {
         Words.push_back(Groups[3]);
         Substr = Substr.substr(Groups[0].size() - Groups[4].size());

clang-tidy/readability/NamespaceCommentCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
-                              "namespace( +([a-zA-Z0-9_]+))?\\.? *(\\*/)?$",
-                              llvm::Regex::IgnoreCase),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),
-      SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {}
+      SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {
+  if (auto RegexOrError =
+          llvm::Regex::compile("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
+                               "namespace( +([a-zA-Z0-9_]+))?\\.? *(\\*/)?$",
+                               llvm::Regex::IgnoreCase)) {
+    NamespaceCommentPattern = std::move(*RegexOrError);
+  }
+}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not

include-fixer/find-all-symbols/HeaderMapCollector.h

 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Regex.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace find_all_symbols {
 
-/// \brief HeaderMappCollector collects all remapping header files. This maps
+/// \brief HeaderMapCollector collects all remapping header files. This maps
 /// complete header names or header name regex patterns to header names.
 class HeaderMapCollector {
 public:
   typedef llvm::StringMap<std::string> HeaderMap;
   typedef std::vector<std::pair<const char *, const char *>> RegexHeaderMap;
 
   HeaderMapCollector() : RegexHeaderMappingTable(nullptr) {}
 

include-fixer/find-all-symbols/HeaderMapCollector.cpp

 HeaderMapCollector::getMappedHeader(llvm::StringRef Header) const {
   auto Iter = HeaderMappingTable.find(Header);
   if (Iter != HeaderMappingTable.end())
     return Iter->second;
   // If there is no complete header name mapping for this header, check the
   // regex header mapping.
   if (RegexHeaderMappingTable) {
     for (const auto &Entry : *RegexHeaderMappingTable) {
-      if (llvm::Regex(Entry.first).match(Header))
+      // All regexes should come from static string constants.
+      if (llvm::ExitOnError(Entry.first)(llvm::Regex::compile(Entry.first))
+              .match(Header)) {
         return Entry.second;
       }
     }
+  }
   return Header;
 }
 
 } // namespace find_all_symbols
 } // namespace clang

include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp

static const HeaderMapCollector::RegexHeaderMap STLPostfixHeaderMap = {
       {"bits/timex.h$", "<ctime>"},
       {"bits/typesizes.h$", "<cstdio>"},
       {"bits/wchar.h$", "<cwchar>"},
       {"bits/wordsize.h$", "<csetjmp>"},
       {"bits/xopen_lim.h$", "<climits>"},
       {"include/xlocale.h$", "<cstring>"},
       {"bits/atomic_word.h$", "<memory>"},
       {"bits/basic_file.h$", "<fstream>"},
-      {"bits/c++allocator.h$", "<string>"},
-      {"bits/c++config.h$", "<iosfwd>"},
-      {"bits/c++io.h$", "<ios>"},
-      {"bits/c++locale.h$", "<locale>"},
+      {"bits/c\\+\\+allocator.h$", "<string>"},
+      {"bits/c\\+\\+config.h$", "<iosfwd>"},
+      {"bits/c\\+\\+io.h$", "<ios>"},
+      {"bits/c\\+\\+locale.h$", "<locale>"},
       {"bits/cpu_defines.h$", "<iosfwd>"},
       {"bits/ctype_base.h$", "<locale>"},
       {"bits/cxxabi_tweaks.h$", "<cxxabi.h>"},
       {"bits/error_constants.h$", "<system_error>"},
       {"bits/gthr-default.h$", "<memory>"},
       {"bits/gthr.h$", "<memory>"},
       {"bits/opt_random.h$", "<random>"},
       {"bits/os_defines.h$", "<iosfwd>"},