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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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.
+  {
----------------
kadircet wrote:
> 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.
The intention was to do less copies: normally there are less diagnostics in preamble than in the main file, so we would add the main file diagnostics first and later **prepend** the preamble diagnostics.

Performance-wise it does not matter and the new version reads better as it only appends and the order of items is natural.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:399
+  // Do not forget to emit a pending diagnostic if there is one.
+  flushLastDiag();
+
----------------
kadircet wrote:
> I suppose we can get rid of the one in `StoreDiags::EndSourceFile` now ?
I've removed the call to `flushDiag()` from the function, but kept the function to reset the `LangOpts` to `None` for symmetry with setting them on `BeginSourceFile`.
Don't think this matters in practice, looks a bit better aesthetically (we "clean up" when the file goes out of scope).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66759