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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks, this looks like in a good shape. I left comments with some thoughts and nits, but they're not blockers, feel free to land it.



================
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> {
----------------
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).


================
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".
----------------
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.
```


================
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, "");
----------------
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();
```


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1786
+
+       if (auto X = cond) {
+       $init[[}]]
----------------
nit: add a case `if (int i = 0; i > 10) { ... }`.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:1859
+      ExpectedHint{" // while \"foo\"", "string"},
+      ExpectedHint{" // while \"...\"", "string_long"},
+      ExpectedHint{" // while true", "boolean"},
----------------
nit: I guess showing `foo...` is better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155421/new/

https://reviews.llvm.org/D155421