[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