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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Sorry for the long delay. This is getting closer. A few more comments though.


================
Comment at: clang-tidy/ClangTidyOptions.cpp:224
@@ +223,3 @@
+      DefaultOptionsProvider::getRawOptions(FileName);
+  OptionsSource CommandLineOptions =
+      OptionsSource(OverrideOptions, OptionsSourceTypeCheckCommandLineOption);
----------------
s/ = \s*OptionsSource//

================
Comment at: clang-tidy/ClangTidyOptions.cpp:235
@@ +234,3 @@
+    if (Iter != CachedOptions.end()) {
+      RawOptions.push_back(Iter->second);
+      break;
----------------
This seems to be changing the caching logic. Consider this directory structure:

  a/
     .clang-tidy
     b/
        c/

And consequtive `getRawOptions` calls with:
  1. "a/some_file"
  2. "a/b/c/some_file"
  3. "a/b/some_file".

What would happen previously:
  1. after call 1 `CachedOptions` would contain an entry for "a"
  2. call 2 would find an entry for "a" and copy it for "a/b" and "a/b/c"
  3. call 3 would just use the cache entry for "a/b"

Now step 2 doesn't copy the cache entry to "a/b" and "a/b/c".

Is there any specific reason to change this? This is benign given that the lookups happen in memory, but then the code needs to be consistent and avoid replicating cache entries to intermediate directories in all cases.

================
Comment at: clang-tidy/ClangTidyOptions.h:102
@@ -101,1 +101,3 @@
 public:
+  static constexpr char OptionsSourceTypeDefaultBinary[] = "clang-tidy binary";
+  static constexpr char OptionsSourceTypeCheckCommandLineOption[] =
----------------
As we figured out with another patch, `constexpr` is not supported by VS2013. Sorry for pushing you in a wrong direction.

================
Comment at: clang-tidy/ClangTidyOptions.h:115
@@ +114,3 @@
+  //
+  /// clang-tidy has 3 types of the sources (from low to top):
+  ///    * clang-tidy binary
----------------
s/from low to top/in order of increasing priority/

================
Comment at: clang-tidy/ClangTidyOptions.h:117
@@ +116,3 @@
+  ///    * clang-tidy binary
+  ///    * '-config' commandline option or a specific configuration file
+  ///    * '-checks' commandline option
----------------
This item should make it clear how a config file relates to the '-config' option in terms of priority. The easiest way to make the order clear is probably to split the item in two (and update the "3 types of the sources" above).

================
Comment at: clang-tidy/ClangTidyOptions.h:127
@@ -108,2 +126,3 @@
   /// specified \p FileName.
-  virtual ClangTidyOptions getOptions(llvm::StringRef FileName) = 0;
+  ClangTidyOptions getOptions(llvm::StringRef FileName) {
+    ClangTidyOptions Result;
----------------
Let's move the implementation to the .cpp file. It's only used by a small fraction of translation units including this header, and it doesn't seem to be performance-critical to make this method inline.

================
Comment at: clang-tidy/ClangTidyOptions.h:145
@@ -121,4 +144,3 @@
   }
-  ClangTidyOptions getOptions(llvm::StringRef /*FileName*/) override {
-    return DefaultOptions;
-  }
+  std::vector<OptionsSource> getRawOptions(
+      llvm::StringRef FileName) override;
----------------
clang-format?

A few more instances below.

================
Comment at: clang-tidy/ClangTidyOptions.h:161
@@ +160,3 @@
+                        const ClangTidyOptions &OverrideOptions);
+  std::vector<OptionsSource> getRawOptions(
+      llvm::StringRef FileName) override;
----------------
clang-format?

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:320
@@ +319,3 @@
+  if (ExplainConfig) {
+    // FIXME: Figure out a more elegant way to show other ClangTidyOptions'
+    // fields like ExtraArg.
----------------
"a more elegant way" seems to suggest that currently this already happens, but in a not elegant fashion. As far as I understand, we're not doing this at all ;)


http://reviews.llvm.org/D18694