[PATCH] D155421: [clangd] Add BlockEnd comments for control flow statements
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rGee032bccc934: [clangd] Add BlockEnd comments for control
flow statements (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D155421?vs=540868=543055#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155421/new/
https://reviews.llvm.org/D155421
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -17,6 +17,8 @@
#include "llvm/Support/ScopedPrinter.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include
+#include
namespace clang {
namespace clangd {
@@ -1757,6 +1759,221 @@
ExpectedHint{" // enum class E2", "E2"});
}
+TEST(BlockEndHints, If) {
+ assertBlockEndHints(
+ R"cpp(
+void foo(bool cond) {
+ if (cond)
+ ;
+
+ if (cond) {
+ $simple[[}]]
+
+ if (cond) {
+ } else {
+ $ifelse[[}]]
+
+ if (cond) {
+ } else if (!cond) {
+ $elseif[[}]]
+
+ if (cond) {
+ } else {
+ if (!cond) {
+ $inner[[}]]
+ $outer[[}]]
+
+ if (auto X = cond) {
+ $init[[}]]
+
+ if (int i = 0; i > 10) {
+ $init_cond[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // if cond", "simple"},
+ ExpectedHint{" // if cond", "ifelse"}, ExpectedHint{" // if", "elseif"},
+ ExpectedHint{" // if !cond", "inner"},
+ ExpectedHint{" // if cond", "outer"}, ExpectedHint{" // if X", "init"},
+ ExpectedHint{" // if i > 10", "init_cond"});
+}
+
+TEST(BlockEndHints, Loops) {
+ assertBlockEndHints(
+ R"cpp(
+void foo() {
+ while (true)
+ ;
+
+ while (true) {
+ $while[[}]]
+
+ do {
+ } while (true);
+
+ for (;true;) {
+ $forcond[[}]]
+
+ for (int I = 0; I < 10; ++I) {
+ $forvar[[}]]
+
+ int Vs[] = {1,2,3};
+ for (auto V : Vs) {
+ $foreach[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while true", "while"},
+ ExpectedHint{" // for true", "forcond"},
+ ExpectedHint{" // for I", "forvar"},
+ ExpectedHint{" // for V", "foreach"});
+}
+
+TEST(BlockEndHints, Switch) {
+ assertBlockEndHints(
+ R"cpp(
+void foo(int I) {
+ switch (I) {
+case 0: break;
+ $switch[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // switch I", "switch"});
+}
+
+TEST(BlockEndHints, PrintLiterals) {
+ assertBlockEndHints(
+ R"cpp(
+void foo() {
+ while ("foo") {
+ $string[[}]]
+
+ while ("foo but this time it is very long") {
+ $string_long[[}]]
+
+ while (true) {
+ $boolean[[}]]
+
+ while (1) {
+ $integer[[}]]
+
+ while (1.5) {
+ $float[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while \"foo\"", "string"},
+ ExpectedHint{" // while \"foo but...\"", "string_long"},
+ ExpectedHint{" // while true", "boolean"},
+ ExpectedHint{" // while 1", "integer"},
+ ExpectedHint{" // while 1.5", "float"});
+}
+
+TEST(BlockEndHints, PrintRefs) {
+ assertBlockEndHints(
+ R"cpp(
+namespace ns {
+ int Var;
+ int func();
+ struct S {
+int Field;
+int method() const;
+ }; // suppress
+} // suppress
+void foo() {
+ while (ns::Var) {
+ $var[[}]]
+
+ while (ns::func()) {
+ $func[[}]]
+
+ while (ns::S{}.Field) {
+ $field[[}]]
+
+ while (ns::S{}.method()) {
+ $method[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while Var", "var"},
+ ExpectedHint{" // while func", "func"},
+ ExpectedHint{" // while Field", "field"},
+ ExpectedHint{" // while method", "method"});
+}
+
+TEST(BlockEndHints, PrintConversions) {
+ assertBlockEndHints(
+ R"cpp(
+struct S {
+ S(int);
+ S(int, int);
+ explicit operator bool();
+}; // suppress
+void foo(int I) {
+ while (float(I)) {
+ $convert_primitive[[}]]
+
+ while (S(I)) {
+ $convert_class[[}]]
+
+ while (S(I, I)) {
+ $construct_class[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while float", "convert_primitive"},
+ ExpectedHint{" // while S", "convert_class"},
+ ExpectedHint{" // while S", "construct_class"});
+}
+
+TEST(BlockEndHints, PrintOperators) {
+ std::string AnnotatedCode = R"cpp(
+void foo(Integer I) {
+ while(++I){
+ $preinc[[}]]
+
+ while(I++){
+ $postinc[[}]]
+
+ while(+(I + I)){
+ $unary_complex[[}]]
+
+ while(I < 0){
+ $compare[[}]]
+
+
[PATCH] D155421: [clangd] Add BlockEnd comments for control flow statements
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 {
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(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
[PATCH] D155421: [clangd] Add BlockEnd comments for control flow statements
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 {
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(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
[PATCH] D155421: [clangd] Add BlockEnd comments for control flow statements
sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
These mark the end of CompoundStmts bodies of if/while/for/switch.
To identify which statement is being ended, we include abbreviated
text of the condition/loop variable.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155421
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -17,6 +17,8 @@
#include "llvm/Support/ScopedPrinter.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include
+#include
namespace clang {
namespace clangd {
@@ -1757,6 +1759,218 @@
ExpectedHint{" // enum class E2", "E2"});
}
+TEST(BlockEndHints, If) {
+ assertBlockEndHints(
+ R"cpp(
+void foo(bool cond) {
+ if (cond)
+ ;
+
+ if (cond) {
+ $simple[[}]]
+
+ if (cond) {
+ } else {
+ $ifelse[[}]]
+
+ if (cond) {
+ } else if (!cond) {
+ $elseif[[}]]
+
+ if (cond) {
+ } else {
+ if (!cond) {
+ $inner[[}]]
+ $outer[[}]]
+
+ if (auto X = cond) {
+ $init[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // if cond", "simple"},
+ ExpectedHint{" // if cond", "ifelse"}, ExpectedHint{" // if", "elseif"},
+ ExpectedHint{" // if !cond", "inner"},
+ ExpectedHint{" // if cond", "outer"}, ExpectedHint{" // if X", "init"});
+}
+
+TEST(BlockEndHints, Loops) {
+ assertBlockEndHints(
+ R"cpp(
+void foo() {
+ while (true)
+ ;
+
+ while (true) {
+ $while[[}]]
+
+ do {
+ } while (true);
+
+ for (;true;) {
+ $forcond[[}]]
+
+ for (int I = 0; I < 10; ++I) {
+ $forvar[[}]]
+
+ int Vs[] = {1,2,3};
+ for (auto V : Vs) {
+ $foreach[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while true", "while"},
+ ExpectedHint{" // for true", "forcond"},
+ ExpectedHint{" // for I", "forvar"},
+ ExpectedHint{" // for V", "foreach"});
+}
+
+TEST(BlockEndHints, Switch) {
+ assertBlockEndHints(
+ R"cpp(
+void foo(int I) {
+ switch (I) {
+case 0: break;
+ $switch[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // switch I", "switch"});
+}
+
+TEST(BlockEndHints, PrintLiterals) {
+ assertBlockEndHints(
+ R"cpp(
+void foo() {
+ while ("foo") {
+ $string[[}]]
+
+ while ("foo but this time it is very long") {
+ $string_long[[}]]
+
+ while (true) {
+ $boolean[[}]]
+
+ while (1) {
+ $integer[[}]]
+
+ while (1.5) {
+ $float[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while \"foo\"", "string"},
+ ExpectedHint{" // while \"...\"", "string_long"},
+ ExpectedHint{" // while true", "boolean"},
+ ExpectedHint{" // while 1", "integer"},
+ ExpectedHint{" // while 1.5", "float"});
+}
+
+TEST(BlockEndHints, PrintRefs) {
+ assertBlockEndHints(
+ R"cpp(
+namespace ns {
+ int Var;
+ int func();
+ struct S {
+int Field;
+int method() const;
+ }; // suppress
+} // suppress
+void foo() {
+ while (ns::Var) {
+ $var[[}]]
+
+ while (ns::func()) {
+ $func[[}]]
+
+ while (ns::S{}.Field) {
+ $field[[}]]
+
+ while (ns::S{}.method()) {
+ $method[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while Var", "var"},
+ ExpectedHint{" // while func", "func"},
+ ExpectedHint{" // while Field", "field"},
+ ExpectedHint{" // while method", "method"});
+}
+
+TEST(BlockEndHints, PrintConversions) {
+ assertBlockEndHints(
+ R"cpp(
+struct S {
+ S(int);
+ S(int, int);
+ explicit operator bool();
+}; // suppress
+void foo(int I) {
+ while (float(I)) {
+ $convert_primitive[[}]]
+
+ while (S(I)) {
+ $convert_class[[}]]
+
+ while (S(I, I)) {
+ $construct_class[[}]]
+} // suppress
+ )cpp",
+ ExpectedHint{" // while float", "convert_primitive"},
+ ExpectedHint{" // while S", "convert_class"},
+ ExpectedHint{" // while S", "construct_class"});
+}
+
+TEST(BlockEndHints, PrintOperators) {
+ std::string AnnotatedCode = R"cpp(
+void foo(Integer I) {
+ while(++I){
+ $preinc[[}]]
+
+ while(I++){
+ $postinc[[}]]
+
+ while(+(I + I)){
+ $unary_complex[[}]]
+
+ while(I < 0){
+ $compare[[}]]
+
+ while((I + I) < I){
+ $lhs_complex[[}]]
+
+ while(I < (I + I)){
+
