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
