[PATCH] D155421: [clangd] Add BlockEnd comments for control flow statements

[Sam McCall via Phabricator via cfe-commits]

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