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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL 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
----------------
njames93 wrote:
> 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.
> 
No, I need both Errors and Names. Not one of them.
And I think that some usages ignore Errors even if they exist.


================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:528
+
+static llvm::StringSet<> findSourcesForDiagnostic(
+    const std::vector<ClangTidyOptionsProvider::OptionsSource> &RawOptions,
----------------
njames93 wrote:
> 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.
--verify-config checks all config files, and prints diagnostic for all of them with a path of file.
In this way if I will have same invalid value in 2 different files, even that one override second, it will still provide warning for both.
No need to limit only to last entry...


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


================
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) {
----------------
njames93 wrote:
> `CheckOptions` is a `StringMap`, so key lookup should be used instead of iterating through all the entries
Ack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146842