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