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