[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

Just to clarify: we only want to surface errors, not warnings from command-line to avoid too much nosie; I'm totally on board with not spamming users with "unknown compiler warning" errors at the start of each file.
We would only show things like `unknown argument "-mdisable-fp-elim"`. Given that clang understands most flags from other compilers (to be a good drop-in replacement), we should end up showing errors when things actually can be significantly broken.

And we will now show this errors only in cases when we could not build the AST at all before (as `CompilerInvocation` was null), so overall we should not be producing too much new noise



================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:465
+    Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
+  // Finally, add diagnostics coming from the AST.
+  {
----------------
kadircet wrote:
> could you preserve the previous order by inserting `CompilerInvocationDiags` here instead of the ones in `AST` ?
Do you think we should add `CompilerInvocatioDiags` at the end, rather than at the start of the diagnostics list?
Why would that be better?

Mostly trying to keep the chronological order here: command-line diagnostics came first, followed by preamble, followed by AST diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66759