[PATCH] D77085: [clang-tidy] Added support for validating configuration options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this, I think it's going to be a very useful interface!



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:25
+  llvm::raw_svector_ostream Output(Buffer);
+  Output << "warning: Option not found '" << OptionName << "'\n";
+  return std::string(Buffer);
----------------
I think the diagnostic text should probably not start with a capital letter. Also, the name of the classes are confusing in that they say error but the diagnostic is a warning. When I hear "error", the class name makes me think this would stop compilation and give a nonzero result code from the program.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:108-113
+  if (CheckGlobal && Iter == CheckOptions.end()) {
+    Iter = CheckOptions.find(LocalName.str());
+  }
+  if (Iter == CheckOptions.end()) {
+    return llvm::make_error<MissingOptionError>((NamePrefix + LocalName).str());
+  }
----------------
Elide braces


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:122-124
+    } else if (Value.equals(NameAndEnum.first))
+      return NameAndEnum.second;
+    else if (Value.equals_lower(NameAndEnum.first)) {
----------------
Preference to add the braces to this case because the surrounding ones do as well.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:41
+public:
+  MissingOptionError(std::string OptionName) : OptionName(OptionName) {}
+
----------------
Make the constructor `explicit`? (May want to consider the same for the other ctors, but this one seems more dangerous.)


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:197
+    std::string get(StringRef LocalName, StringRef Default) const {
+      if (auto Val = get(LocalName))
+        return *Val;
----------------
Don't use `auto` as the type is not spelled out.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:201
+        llvm::consumeError(Val.takeError());
+      return std::string(Default);
+    }
----------------
`Default.str()` instead?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:219-223
+      if (auto Val = getLocalOrGlobal(LocalName))
+        return *Val;
+      else
+        llvm::consumeError(Val.takeError());
+      return std::string(Default);
----------------
Same here as above.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:280
+        ValueOr = getLocalOrGlobal(LocalName);
+        if (!ValueOr) {
+          return std::move(ValueOr.takeError());
----------------
Elide braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77085