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