sammccall accepted this revision. sammccall added inline comments.
================ Comment at: clangd/tool/ClangdMain.cpp:204 +static llvm::cl::opt<std::string> ClangTidyChecks( + "clang-tidy-checks", ---------------- hokein wrote: > sammccall wrote: > > Maybe add a TODO or FIXME to respect .clang-tidy files? > didn't get the point of the comment -- in this patch, clangd will read > configurations from `.clang-tidy` files (`FileOptionsProvider` provides this > functionality). This command-line flag is used to overwrite the `.clang-tidy` > configurations,. Ah, I missed that FileOptionsProvider was actually used. Can you update the desc to something like "List of clang-tidy checks to run (overrides .clang-tidy files)"? ================ Comment at: clangd/tool/ClangdMain.cpp:435 + RealFileSystemProvider FSProvider; + // Create an empty option. ---------------- please move this up to a section near the other feature configuration stuff. e.g. below where CCOpts is initialized. The Transport/ClangdLSPServer initialization is more closely related. ================ Comment at: clangd/tool/ClangdMain.cpp:438 + auto OverrideClangTidyOptions = tidy::ClangTidyOptions::getDefaults(); + if (!ClangTidyChecks.empty()) + OverrideClangTidyOptions.Checks = ClangTidyChecks; ---------------- This prevents `-clang-tidy-checks=` from disabling all checks. use ClangTidyChecks.getNumOccurrences() instead? ================ Comment at: unittests/clangd/TestTU.cpp:39 + Inputs.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults(); + Inputs.ClangTidyOpts.Checks = + "-*, bugprone-sizeof-expression, bugprone-macro-repeated-side-effects, " ---------------- Make this list an optional attribute to TestTU instead of hard-coding? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55256/new/ https://reviews.llvm.org/D55256 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
