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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks LGTM!



================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:107
+
+    Cmd = CDB->getFallbackCommand(File);
+    if (auto TrueCmd = CDB->getCompileCommand(File)) {
----------------
already done in the else statement.


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:125
+    Inputs.Opts.ClangTidyOpts =
+        Opts.GetClangTidyOptions(*TFS.view(llvm::None), File);
+    log("Parsing command...");
----------------
sammccall wrote:
> 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.
ah sorry, i forgot `Inputs` was a member


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:131
+    showErrors(InvocationDiags);
+    log("cc1 args are: {0}", llvm::join(CC1Args, " "));
+    if (!Invocation) {
----------------
sammccall wrote:
> 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?
SG, and actually thinking again, we are only making changes in the positive direction for clangd. i.e. there shouldn't be an instance in which this cc1 works, but clangd fails.


================
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,
----------------
sammccall wrote:
> 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?
yeah, it just felt uncommon, i don't think we can mess this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88338