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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
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.
+  {
----------------
ilya-biryukov wrote:
> 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.
I totally agree that your current ordering makes more sense. I just wasn't sure why it was done in the opposite way before(first AST, then preamble), if it doesn't cause any test breakages we should be good though.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:603
     FillDiagBase(*LastDiag);
     LastDiagWasAdjusted = false;
     if (!InsideMainFile)
----------------
I suppose this one is also not required anymore, as you clear it during flush


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:755
+
+  EXPECT_THAT(
+      Diagnostics,
----------------
as discussed offline, could you also add a test to make sure this does not fire on `"-Wunknown-warning-flag"` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66759