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

Daniel Jasper djasper at google.com
Thu Sep 4 05:39:11 PDT 2014


================
Comment at: clang-tidy/ClangTidyOptions.cpp:102
@@ +101,3 @@
+
+const ClangTidyOptions &FileOptionsProvider::getOptions(StringRef FileName) {
+  DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n");
----------------
alexfh wrote:
> 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 ;)
Absolutely, add a FIXME.

FWIW, it might makes sense to just have a function called findParentFileWithName or something that only encapsulates the functionality of walking up directories. Here, this will be sufficient. For clang-format that can then be called in a loop if a certain file is found but unusable.

================
Comment at: clang-tidy/ClangTidyOptions.cpp:83
@@ +82,3 @@
+
+  if (Other.HeaderFilterRegex)                                                             \
+    Result.HeaderFilterRegex = Other.HeaderFilterRegex;
----------------
I think you want to remove the escaped newline.

================
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."),
----------------
alexfh wrote:
> 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.
This can't be the current behavior as currently, there are no .clang-tidy files. And that's also why I want us to think about it now, before we have to break existing behavior. In your opinion, what are the pros/cons for/against appending?

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:91
@@ +90,3 @@
+
+static cl::opt<bool> AnalyzeTemporaryDtors(
+    "analyze-temporary-dtors",
----------------
alexfh wrote:
> 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.
Ok, but what are the more frequently used flags? Also, dumping the current config into a .clang-tidy file and changing that seems like the more convenient way to change these things anyway.

FWIW, I somewhat agree on keeping "-checks" (although I have no data on how frequently it's used). However, I don't thing -analyze-temporary-dtors is or should be a "frequent flag", which is why I left the comment here.

http://reviews.llvm.org/D5186






More information about the cfe-commits mailing list