[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=<file-path>" to specify custom config file
Dmitry Polukhin via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 26 03:15:31 PDT 2020
DmitryPolukhin added a comment.
I'm not sure that we need additional option to read configuration from file but, if we do need, I think this diff needs some improvements + test for new option.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:324
+ llvm::sys::fs::is_symlink_file(Twine(AbsoluteFilePath), IsLink);
+ if (!(IsFile || IsLink)) {
+ std::string Msg;
----------------
Is it actually required to check absolute path, link it or not, etc.? Why not just try reading file with provided filename and report error if it fails?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:408
+FileOptionsProvider::tryReadConfigFile(StringRef Path, bool IsFile) {
+ // llvm::outs() << "tryReadConfigFile IsFile<" <<
+ // OverrideOptions.ClangTidyConfig << ">\n";
----------------
It seems like some debug prints.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:68
+ /// Clang-tidy-config
+ llvm::Optional<std::string> ClangTidyConfig;
+
----------------
I'm not sure that we need it here. I would reuse code path for `--config` options as much as possible and implement new option as a simple wrapper that reads content of the file and interpret it as `--config` option. Moreover I think it should not be possible to specify both options in command line.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:233
llvm::Optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
+ llvm::Optional<OptionsSource> tryReadConfigFile(llvm::StringRef Path,
+ bool IsFile);
----------------
It looks like the second argument was added only for overload resolution. But I think it is better to rename this function. After all it is not "try" anymore because it reports fatal error in case of errors.
================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:76
+static cl::opt<std::string> ClangTidyConfig("clang-tidy-config", cl::desc(R"(
+Specify full path of .clang-tidy config file.
----------------
I would call it something like `--config-file`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89936/new/
https://reviews.llvm.org/D89936
More information about the cfe-commits
mailing list