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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:87
+    auto Mangler = CommandMangler::detect();
+    // Check for missing resource dir?
+    if (Opts.ResourceDir)
----------------
kadircet wrote:
> 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.
Moved FIXME to the right place.


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+    Inputs.Opts.ClangTidyOpts =
+        Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+    log("Parsing command...");
----------------
kadircet wrote:
> IIRC, option providers don't really log anything about success/failure of parsing the config file. what's the point of having this exactly?
This is needed in order to run the correct clang-tidy checks.


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:126
+        Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+    log("Parsing command...");
+    Invocation =
----------------
kadircet wrote:
> 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?
I don't think that's clear - you can find it out by reading the code, but I expect people to be running this who aren't reading the code. (They won't be able to work out all the details, but they tend to be interested in the broad strokes of what's going on).

So generally I've tried to include log statements before each major step.


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+    showErrors(InvocationDiags);
+    log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+    if (!Invocation) {
----------------
kadircet wrote:
> 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.
Sure, and we also run clang-tidy, and fiddle with the preamble, and skip function bodies...

I think a disclaimer that "running clangd is not equivalent to running clang <cc1-args>" would be somewhat confusing, as I'm not sure that's a thing that anyone would expect to be true.

I expect the cc1 args to be useless to anyone not familiar with clangd internals, but they're kind of important to include. Reworded the log message a bit, is this enough?


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:202
+      }
+      unsigned Definitions = locateSymbolAt(*AST, Pos, &Index).size();
+      vlog("    definition: {0}", Definitions);
----------------
kadircet wrote:
> maybe we could print errors if the following has no results for "identifiers" ?
There are lots of ways go-to-def can yield no results (e.g. macros, #ifdef-'d sections, templated code we can't resolve)


================
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,
----------------
kadircet wrote:
> why not have this in `Check.h` ?
A header seems a bit out of place in tool/ and it doesn't seem necessary, being one function with a simple signature and no unit tests.

We'll get a linker error if we ever get this wrong, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88338/new/

https://reviews.llvm.org/D88338