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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits