[PATCH] D88338: [clangd] clangd --check: standalone diagnosis of common problems

[Kadir Cetinkaya via Phabricator via cfe-commits]

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