sammccall marked 3 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:252
+// This is used to summarize e.g. the condition of a while loop.
+std::string summarizeExpr(const Expr *E) {
+ struct Namer : ConstStmtVisitor<Namer, std::string> {
----------------
hokein wrote:
> This looks like a complicated implementation to get an abbreviated-form of
> cond expression for the inlay label text.
>
>
> A different idea would be if the user can just click the inlay label text,
> then the editor jumps to the corresponding for statement (looks like it is
> supported with the
> [InlayHintLabelPart::Location](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHintLabelPart))
> with that we probably don't need to emit an abbreviated-form text (`// for`
> should be clear enough).
> A different idea would be if the user can just click the inlay label text,
> then the editor jumps to the corresponding for statement (looks like it is
> supported with the InlayHintLabelPart::Location) with that we probably don't
> need to emit an abbreviated-form text (// for should be clear enough).
Agree we should have this too (also for other kinds of hints).
But I don't think it's a replacement:
- BlockEnd is most useful to connect blockend + blockstart context when the
block start is far away, and navigating to it destroys your blockend context.
(Hover is better in this regard but still isn't available *in-line*).
- inlayhintlabelpart isn't available in most editors that support inlayhint,
and realistically may never be
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:578
+ // } else if (cond2) {
+ // } // mark as "cond1" or "cond2"?
+ // For now, the answer is neither, just mark as "if".
----------------
hokein wrote:
> my opinion for this case would be (the marking result is also consistent with
> the brackets)
>
> ```
> if (cond1) {
> } // mark as cond1
> else if (cond2) {
> } // mark as cond2.
> ```
If the first if is } is on a separate line we could do this.
The styles we work the most place `} else` on one line though, so this isn't an
option.
In those styles we see if/elseif/else as a single construct (and format it as
such) - and marking as cond2 seems strange to me...
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:599
+ llvm::StringRef Name = "") {
+ if (const auto *CS = llvm::dyn_cast_or_null<CompoundStmt>(Body))
+ addBlockEndHint(CS->getSourceRange(), Label, Name, "");
----------------
hokein wrote:
> it looks like we will mark the block end for single-statement `CompoundStmt`
> without `{}`, it doesn't seem to add much value to this case (the body is
> short enough to spot the for statement).
>
> ```
> for (int i = 0; i < 10; ++i)
> foo();
> ```
As discussed offline, CompoundStmt exists exactly where braces exist.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155421/new/
https://reviews.llvm.org/D155421
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
