[PATCH] D44142: [clangd] Revamp handling of diagnostics.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:329
+          {"title",
+           llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
           {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
----------------
sammccall wrote:
> nit: while here, can we add a colon after FixIt (or if appropriate in practice, just drop the prefix altogether?). My recollection is this is a bit hard to parse.
I added a colon for now, but happy to look into removing the prefix. The use-case that's slightly confusing is:

```use of undeclared identifier 'foo'. did you mean 'bar'?```
Since the actual fix-it is at the end, it's might be a bit hard to understand.


================
Comment at: clangd/Diagnostics.cpp:200
+
+  DiagWithFixIts Main = PreBuild(D.main());
+  Main.Diag.message = presentMainMessage(D);
----------------
sammccall wrote:
> (nit: if the goal with the callback function is to avoid allocations, shouldn't we reuse the DiagWithFixIts?)
Can we actually avoid allocating some of the strings there?
We're `std::move`ing the result into the callback, so we don't seem to waste memory too.


================
Comment at: clangd/Diagnostics.h:58
+/// by fields of PersistentDiag.
+class DiagList {
+public:
----------------
sammccall wrote:
> This is cool :) but really looks like premature optimization.
> Particularly, if the diagnostics were a self-contained value type, this could just be vector<diag> I think. And if we care about copying strings (I'm not sure we do), being lifetime-scoped to the ASTcontext is probably OK.
> 
> (rawDiags() has potential uses but having callers explicitly traverse is going to be clearer as your comment suggests)
Using a straight-forward representation for now. 


================
Comment at: clangd/Diagnostics.h:123
+/// still be included as part of their main diagnostic's message.
+void toLSPDiags(const DiagList &Diags,
+                llvm::function_ref<void(DiagWithFixIts)> OutFn);
----------------
sammccall wrote:
> I don't think this overload pays for itself - making callers write the outer loop would be a little clearer, I think.
> 
> (I'd consider having that fn return a smallvector instead of taking a callback, but don't feel strongly)
Still using a callback, but happy to return a `SmallVector` instead. It seems the pattern for using `SmallVector` in llvm is passing it in as an output parameter (so that the users might decide themselves what's the count of items on the stack), which is slightly ugly.
It might be fine to return `SmallVector` here though, this function can probably make assumptions about the number of notes in a diagnostic.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44142