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

Alexander Kornienko alexfh at google.com
Thu Sep 4 05:28:49 PDT 2014


================
Comment at: clang-tidy/ClangTidyOptions.cpp:83
@@ +82,3 @@
+
+#define MERGE_REPLACE(field)                                                   \
+  if (Other.field)                                                             \
----------------
djasper wrote:
> So currently, this macro makes the code one line longer.. ;-)..
Removed it for now. When we have more options. it may become useful again.

================
Comment at: clang-tidy/ClangTidyOptions.cpp:102
@@ +101,3 @@
+
+const ClangTidyOptions &FileOptionsProvider::getOptions(StringRef FileName) {
+  DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n");
----------------
djasper wrote:
> 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.
getStyle differs significantly from this one: it handles languages, default styles, unsuitable configurations etc. This could be abstracted away, but I have no confidence that this will simplify anything. I could try to do this separately from this patch, if you don't mind, so we don't break two tools simultaneously ;)

================
Comment at: clang-tidy/ClangTidyOptions.h:54
@@ +53,3 @@
+    ClangTidyOptions Options;
+    Options.Checks = "";
+    Options.HeaderFilterRegex = "";
----------------
djasper wrote:
> Did the default change from "*" to ""? Does that change the behavior?
Yes, but this is a very basic default. It was never used in the command-line use case - there's a separate bunch of defaults in the tool's command-line options. All the code relying on the old default here (unit tests) has been cleaned up.

================
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");
----------------
djasper wrote:
> Nit: "The effective configuration. .." ?
Done.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:37
@@ -29,3 +36,3 @@
 
 const char DefaultChecks[] =
     "*,"                       // Enable all checks, except these:
----------------
djasper wrote:
> I understand what is happening here, but now it kind of seems that we set default config values at two places.
The other one (ClangTidyOptions::getDefaults()) is basically a safeguard against unconfigured values for non-command-line use cases (unit tests, etc.).

This one is what the tool actually uses when run from the command line without arguments and finds no .clang-tidy files. There was a plan to remove these defaults and use project-wide .clang-tidy files instead, but I would like to add .clang-tidy files to all relevant projects first.

================
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."),
----------------
djasper wrote:
> 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.
This is done to keep the current behavior. If we want to change it, I'd rather do this as a separate step. And we also need strong reasons to do so, as it would degrade user experience, imo.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:91
@@ +90,3 @@
+
+static cl::opt<bool> AnalyzeTemporaryDtors(
+    "analyze-temporary-dtors",
----------------
djasper wrote:
> 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.
I'd keep at least the most frequently used options, e.g. -checks:

  clang-tidy -config='{Checks: "..."}'

instead of

  clang-tidy -checks="..."

is not extremely ugly, but a bit less convenient. Anyways, I'd like to make any unnecessary behavior changes in a separate patch.

http://reviews.llvm.org/D5186






More information about the cfe-commits mailing list