[PATCH] D146842: [clang-tidy] Fix crash when handling invalid config values

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.h:61-69
 struct NamesAndOptions {
   llvm::StringSet<> Names;
   llvm::StringSet<> Options;
+  std::vector<ClangTidyError> Errors;
 };
 
 NamesAndOptions
----------------
I don't think this is the correct approach here
`getAllChecksAndOptions` should instead return an `llvm::Expected<NamesAndOptions>`.
You can create an error class that wraps a `std::vector<ClangTidyError>` and then still get the same behaviour on the error path.



================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:528
+
+static llvm::StringSet<> findSourcesForDiagnostic(
+    const std::vector<ClangTidyOptionsProvider::OptionsSource> &RawOptions,
----------------
Given how options are read, we only care about the last option source that defined the option
Therefore this should be changed to return a `std::optional<StringRef>` and the for loop below can just search the RawOptions backwards for the first match, then return that.


================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:529
+static llvm::StringSet<> findSourcesForDiagnostic(
+    const std::vector<ClangTidyOptionsProvider::OptionsSource> &RawOptions,
+    llvm::StringRef Diagnostic) {
----------------



================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:542
+  for (const auto &[Opts, Source] : RawOptions) {
+    for (const auto &[ItOption, ItValue] : Opts.CheckOptions) {
+      if (ItOption == Option && ItValue.Value == Value) {
----------------
`CheckOptions` is a `StringMap`, so key lookup should be used instead of iterating through all the entries


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146842