[PATCH] Implemented clang-tidy configurability via .clang-tidy files.

Daniel Jasper djasper at google.com
Thu Sep 4 04:44:50 PDT 2014


High-level comments.

================
Comment at: clang-tidy/ClangTidy.h:122
@@ -123,3 +121,3 @@
 ClangTidyStats
-runClangTidy(ClangTidyOptionsProvider *OptionsProvider,
+runClangTidy(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
              const tooling::CompilationDatabase &Compilations,
----------------
.... grumble about this being an unrelated cleanup .. grumble ...

================
Comment at: clang-tidy/ClangTidyOptions.cpp:83
@@ +82,3 @@
+
+#define MERGE_REPLACE(field)                                                   \
+  if (Other.field)                                                             \
----------------
So currently, this macro makes the code one line longer.. ;-)..

================
Comment at: clang-tidy/ClangTidyOptions.cpp:102
@@ +101,3 @@
+
+const ClangTidyOptions &FileOptionsProvider::getOptions(StringRef FileName) {
+  DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n");
----------------
Can we pull out code shared with getStyle() in include/clang/Format/Format.h? Seems bad to have this kind of logic in the codebase twice.

================
Comment at: clang-tidy/ClangTidyOptions.h:54
@@ +53,3 @@
+    ClangTidyOptions Options;
+    Options.Checks = "";
+    Options.HeaderFilterRegex = "";
----------------
Did the default change from "*" to ""? Does that change the behavior?

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:34
@@ +33,3 @@
+    "  file. If any configuration options have a corresponding command-line\n"
+    "  option, command-line option takes precedence. Effective configuration\n"
+    "  can be inspected using -dump-config.\n\n");
----------------
Nit: "The effective configuration. .." ?

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:37
@@ -29,3 +36,3 @@
 
 const char DefaultChecks[] =
     "*,"                       // Enable all checks, except these:
----------------
I understand what is happening here, but now it kind of seems that we set default config values at two places.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:50
@@ +49,3 @@
+                          "set of enabled checks.\n"
+                          "This option's value is appended to the value read\n"
+                          "from a .clang-tidy file, if any."),
----------------
I am not entirely certain that appending is the right thing to do here. But I understand that otherwise, it would be next to impossible to say "everything in the config file +/- this one check". Then again, with -dump-config, it might be easy enough to copy and past the existing active checks for a given file and modify them.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:91
@@ +90,3 @@
+
+static cl::opt<bool> AnalyzeTemporaryDtors(
+    "analyze-temporary-dtors",
----------------
Hm. I am wondering whether it really makes sense to keep these individual config flags. The approach in clang-format, where we can just pass in a JSON config through a single flag seems like a good alternative.

http://reviews.llvm.org/D5186






More information about the cfe-commits mailing list