kadircet added a comment.
thanks! this mostly looks good, as discussed offline I believe having an infra
that we can improve over time is better than not having anything until we've
got the "perfect" solution.
i just got a couple of questions about some pieces, and some nits.
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:57
+// Nonfatal failures are logged and tracked in ErrCount.
+class Checker {
+ // from constructor
----------------
put into anon namespace?
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:87
+ auto Mangler = CommandMangler::detect();
+ // Check for missing resource dir?
+ if (Opts.ResourceDir)
----------------
i agree, this would help identifying the case of "clangd binary has been copied
to some place without the necessary lib directory".
but i think we should check for the final `-resource-dir` in the compile
command below. as user's compile flags can override whatever default clangd
comes up with.
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:95
+ Cmd = CDB->getFallbackCommand(File);
+ log("Generic fallback command is: {0}", llvm::join(Cmd.CommandLine, " "));
+ if (auto TrueCmd = CDB->getCompileCommand(File)) {
----------------
we don't have any custom fallback commands, what's the point of printing this
always?
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+ Inputs.Opts.ClangTidyOpts =
+ Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+ log("Parsing command...");
----------------
IIRC, option providers don't really log anything about success/failure of
parsing the config file. what's the point of having this exactly?
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:126
+ Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+ log("Parsing command...");
+ Invocation =
----------------
it is clear that we've crashed while parsing compile commands if we don't see
`cc1 args are` in the output. so maybe drop this one?
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+ showErrors(InvocationDiags);
+ log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+ if (!Invocation) {
----------------
maybe we should also note that this doesn't reflect the final result. as we
turn off pchs or dependency file outputting related flags afterwards.
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:137
+
+ Style = getFormatStyleForFile(File, Inputs.Contents, TFS);
+
----------------
this seems ... surprising :D but I also don't have suggestion for a better
place.
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:202
+ }
+ unsigned Definitions = locateSymbolAt(*AST, Pos, &Index).size();
+ vlog(" definition: {0}", Definitions);
----------------
maybe we could print errors if the following has no results for "identifiers" ?
================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:214
+ // Print (and count) the error-level diagnostics (warnings are ignored).
+ void showErrors(llvm::ArrayRef<Diag> Diags) {
+ for (const auto &D : Diags) {
----------------
nit: maybe make this a free function and `return ErrCount` ?
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:52
+// Implemented in Check.cpp.
+bool check(const llvm::StringRef File, const ThreadsafeFS &TFS,
+ llvm::Optional<std::string> CompileCommandsPath,
----------------
why not have this in `Check.h` ?
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:833
+ ? 0
+ : (int)ErrorResultCode::CheckFailed;
+ }
----------------
nit: `static_cast<int>(Err..)`
maybe do the same for line 846 while here. (line 872 already does that)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88338/new/
https://reviews.llvm.org/D88338
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
