This adds a support for the .clang-tidy file reading using
FileOptionsProvider, -dump-config option, and changes tests to not depend on
default checks set.
Details
Diff Detail
Event Timeline
High-level comments.
clang-tidy/ClangTidy.h | ||
---|---|---|
122 | .... grumble about this being an unrelated cleanup .. grumble ... | |
clang-tidy/ClangTidyOptions.cpp | ||
83 | So currently, this macro makes the code one line longer.. ;-).. | |
102 | 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. | |
clang-tidy/ClangTidyOptions.h | ||
54 | Did the default change from "*" to ""? Does that change the behavior? | |
clang-tidy/tool/ClangTidyMain.cpp | ||
34 | Nit: "The effective configuration. .." ? | |
37 | I understand what is happening here, but now it kind of seems that we set default config values at two places. | |
50 | 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. | |
91 | 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. |
Addressed review comments.
clang-tidy/ClangTidyOptions.cpp | ||
---|---|---|
83 | Removed it for now. When we have more options. it may become useful again. | |
102 | 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 ;) | |
clang-tidy/ClangTidyOptions.h | ||
54 | 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. | |
clang-tidy/tool/ClangTidyMain.cpp | ||
34 | Done. | |
37 | 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. | |
50 | 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. | |
91 | 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. |
clang-tidy/ClangTidyOptions.cpp | ||
---|---|---|
83 | I think you want to remove the escaped newline. | |
102 | 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. | |
clang-tidy/tool/ClangTidyMain.cpp | ||
50 | 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? | |
91 | 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. |
clang-tidy/ClangTidyOptions.cpp | ||
---|---|---|
83 | Absolutely. | |
clang-tidy/tool/ClangTidyMain.cpp | ||
50 | The current behavior is that adding -checks=... on the command line modifies the set of checks by appending the new value to the value that would be used without the command line option. The new code behaves exactly in the same way when there's no .clang-tidy file, and naturally extends the same behavior to the case when there is a .clang-tidy file (the "defaults" are taken from the .clang-tidy file instead). If instead of appending we overwrite the value, we'll change the behavior of clang-tidy in both use cases. Apart from trying to avoid changes in behavior, I see the following benefit of the current approach: it allows to slightly change the default set of checks as well as completely override it equally easily (use -checks=some-check to add a check to the default set, -checks=-some-check to remove it, -checks=-*,some-check to only run some-check). | |
91 | I totally agree with the idea of removing -analyze-temporary-dtors if we add a command-line option to specify JSON config. The -header-filter= option also doesn't look like a frequently used. However, -checks= is used at least by the check developers and in all our documents, so we'd better leave it. |
.... grumble about this being an unrelated cleanup .. grumble ...