[PATCH] D18694: [ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Nice! I like the idea, but we need to figure out a bunch of details (see the comments inline).


================
Comment at: clang-tidy/ClangTidyOptions.cpp:264
@@ +263,3 @@
+      ParsedOptions->CheckSources[*ParsedOptions->Checks] =
+          std::string(ConfigFile.c_str());
+    }
----------------
`ConfigFile.str()` should also work.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:131
@@ -130,1 +130,3 @@
 
+static cl::opt<bool> ExplainChecks("explain-checks", cl::desc(R"(
+explains where each check comes from, i.e. command line, .clang-tidy
----------------
I would expect something different from an option named '-explain-checks', e.g. some kind of a short documentation on each check or the like. Something like `explain-config` or `describe-config` would be closer to what the option does, I think.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:132
@@ +131,3 @@
+static cl::opt<bool> ExplainChecks("explain-checks", cl::desc(R"(
+explains where each check comes from, i.e. command line, .clang-tidy
+configuration file.
----------------
How about `for each enabled check explains, where it is enabled, i.e. in clang-tidy binary, command line or a specific configuration file`?

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:265
@@ -258,2 +264,3 @@
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.CheckSources[DefaultChecks] = "ClangTidy binary";
   DefaultOptions.WarningsAsErrors = "";
----------------
s/ClangTidy/clang-tidy/

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:278
@@ -270,1 +277,3 @@
     OverrideOptions.Checks = Checks;
+    OverrideOptions.CheckSources[Checks] = "command-line argument 'checks'";
+  }
----------------
s/argument 'checks'/option '-checks'/

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:294
@@ +293,3 @@
+        ParsedConfig->CheckSources[*ParsedConfig->Checks] =
+            "command-line argument 'config'";
+      }
----------------
s/argument 'config'/option '-config'/

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:329
@@ +328,3 @@
+    for (const std::string& Check : EnabledChecks) {
+      for (const ClangTidyOptions::StringPair &CheckSource:
+           EffectiveOptions.CheckSources) {
----------------
I'm not sure I understand how this should work in case a check is:
1. enabled in binary (e.g. `-*,a`)
2. disabled in the config file (e.g. `-a`)
3. enabled again on the command line (e.g. `a`)

clang-tidy should say 'command line' in this case, but i'm not sure what your implementation will do, since it doesn't seem to keep the order of the configurations.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:331
@@ +330,3 @@
+           EffectiveOptions.CheckSources) {
+        GlobList filter(CheckSource.first);
+        if (filter.contains(Check)) {
----------------
nit: Filter

You could also write `if (GlobList(CheckSource.first).contains(Check)) {`

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:333
@@ +332,3 @@
+        if (filter.contains(Check)) {
+          llvm::outs() << "'" << Check << "' comes from " << CheckSource.second
+                       << ".\n";
----------------
s/comes from/is enabled in the/


Repository:
  rL LLVM

http://reviews.llvm.org/D18694