[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG9e83d0bcdfe8: [clangd] Mention when CXXThis is implicit in
exposed AST. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D91868?vs=306681&id=307363#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91868/new/
https://reviews.llvm.org/D91868
Files:
clang-tools-extra/clangd/DumpAST.cpp
clang-tools-extra/clangd/unittests/DumpASTTests.cpp
Index: clang-tools-extra/clangd/unittests/DumpASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/DumpASTTests.cpp
+++ clang-tools-extra/clangd/unittests/DumpASTTests.cpp
@@ -76,29 +76,32 @@
type: Record - S
)"},
{R"cpp(
-template int root() {
- (void)root();
+namespace root {
+template int tmpl() {
+ (void)tmpl();
return T::value;
+}
}
)cpp",
R"(
-declaration: FunctionTemplate - root
- declaration: TemplateTypeParm - T
- declaration: Function - root
-type: FunctionProto
- type: Builtin - int
-statement: Compound
- expression: CStyleCast - ToVoid
-type: Builtin - void
-expression: Call
- expression: ImplicitCast - FunctionToPointerDecay
-expression: DeclRef - root
- template argument: Type
-type: Builtin - unsigned int
- statement: Return
-expression: DependentScopeDeclRef - value
- specifier: TypeSpec
-type: TemplateTypeParm - T
+declaration: Namespace - root
+ declaration: FunctionTemplate - tmpl
+declaration: TemplateTypeParm - T
+declaration: Function - tmpl
+ type: FunctionProto
+type: Builtin - int
+ statement: Compound
+expression: CStyleCast - ToVoid
+ type: Builtin - void
+ expression: Call
+expression: ImplicitCast - FunctionToPointerDecay
+ expression: DeclRef - tmpl
+template argument: Type
+ type: Builtin - unsigned int
+statement: Return
+ expression: DependentScopeDeclRef - value
+specifier: TypeSpec
+ type: TemplateTypeParm - T
)"},
{R"cpp(
struct Foo { char operator+(int); };
@@ -116,10 +119,28 @@
type: Record - Foo
expression: IntegerLiteral - 42
)"},
+ {R"cpp(
+struct Bar {
+ int x;
+ int root() const {
+return x;
+ }
+};
+ )cpp",
+ R"(
+declaration: CXXMethod - root
+ type: FunctionProto
+type: Builtin - int
+ statement: Compound
+statement: Return
+ expression: ImplicitCast - LValueToRValue
+expression: Member - x
+ expression: CXXThis - const, implicit
+ )"},
};
for (const auto &Case : Cases) {
ParsedAST AST = TestTU::withCode(Case.first).build();
-auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "root")),
+auto Node = dumpAST(DynTypedNode::create(findUnqualifiedDecl(AST, "root")),
AST.getTokens(), AST.getASTContext());
EXPECT_EQ(llvm::StringRef(Case.second).trim(),
llvm::StringRef(llvm::to_string(Node)).trim());
Index: clang-tools-extra/clangd/DumpAST.cpp
===
--- clang-tools-extra/clangd/DumpAST.cpp
+++ clang-tools-extra/clangd/DumpAST.cpp
@@ -234,6 +234,14 @@
return UnaryOperator::getOpcodeStr(UO->getOpcode()).str();
if (const auto *CCO = dyn_cast(S))
return CCO->getConstructor()->getNameAsString();
+if (const auto *CTE = dyn_cast(S)) {
+ bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
+ if (CTE->isImplicit())
+return Const ? "const, implicit" : "implicit";
+ if (Const)
+return "const";
+ return "";
+}
if (isa(S) || isa(S) ||
isa(S) || isa(S) ||
isa(S) || isa(S))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.
kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/DumpAST.cpp:233
return CCO->getConstructor()->getNameAsString();
+if (const auto *CTE = dyn_cast(S)) {
+ bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
sammccall wrote:
> kadircet wrote:
> > should we ensure we always return within the branch (as we do within the
> > rest of the branches to make sure we don't accumulate details by mistake)?
> > e.g:
> > ```
> > if(CXXThisExpr) {
> > details = {}
> > if (const) details += "const";
> > if (implicit) details += "implicit";
> > return join(",", details);
> > }
> > ```
> Actually I was trying to *avoid* returning `""` in several places.
> I think of these functions as a collection of special cases where we have
> something to say - if we don't find anything, we fall off the end.
> (`getDetail` for DeducedType is the other example)
>
> Can change it if you think it's important though.
> Actually I was trying to *avoid* returning "" in several places.
I agree that this is bad, but it feels acceptable when the code doesn't read
that way directly :P
> I think of these functions as a collection of special cases where we have
> something to say - if we don't find anything, we fall off the end.
I was afraid of entering other branches, before falling off the end (not
applicable here, but assume one day we have a children of `CXXThisExpr`, this
code will prefer the children when thisexpr is neither const nor implicit,
which might be surprising).
> Can change it if you think it's important though.
As mentioned not an immediate threat, but might become surprising in the future
and require some digging. So would be nice if you can.
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:79
{R"cpp(
-template int root() {
- (void)root();
+namespace root {
+template int tmpl() {
sammccall wrote:
> kadircet wrote:
> > is this change intentional ?
> Yeah, the problem is that I wanted the root in the new test to be a member
> function, so I needed to switch from findDecl("root") to
> findUnqualifiedDecl("root"). But that fails if there are two decls with that
> name, as there were here: the primary template root and the specialization
> root. Thus the change to wrap the whole thing in a root namespace.
> Maybe there's a neater way to solve this, I'm not sure it matters much.
ah makes sense. sorry i missed the change from finddecl to unqualifieddecl
within the test body.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91868/new/
https://reviews.llvm.org/D91868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/DumpAST.cpp:233
return CCO->getConstructor()->getNameAsString();
+if (const auto *CTE = dyn_cast(S)) {
+ bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
kadircet wrote:
> should we ensure we always return within the branch (as we do within the rest
> of the branches to make sure we don't accumulate details by mistake)? e.g:
> ```
> if(CXXThisExpr) {
> details = {}
> if (const) details += "const";
> if (implicit) details += "implicit";
> return join(",", details);
> }
> ```
Actually I was trying to *avoid* returning `""` in several places.
I think of these functions as a collection of special cases where we have
something to say - if we don't find anything, we fall off the end.
(`getDetail` for DeducedType is the other example)
Can change it if you think it's important though.
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:79
{R"cpp(
-template int root() {
- (void)root();
+namespace root {
+template int tmpl() {
kadircet wrote:
> is this change intentional ?
Yeah, the problem is that I wanted the root in the new test to be a member
function, so I needed to switch from findDecl("root") to
findUnqualifiedDecl("root"). But that fails if there are two decls with that
name, as there were here: the primary template root and the specialization
root. Thus the change to wrap the whole thing in a root namespace.
Maybe there's a neater way to solve this, I'm not sure it matters much.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91868/new/
https://reviews.llvm.org/D91868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.
kadircet added a comment.
This looks like an improvement to me as well, thanks!
Comment at: clang-tools-extra/clangd/DumpAST.cpp:233
return CCO->getConstructor()->getNameAsString();
+if (const auto *CTE = dyn_cast(S)) {
+ bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
should we ensure we always return within the branch (as we do within the rest
of the branches to make sure we don't accumulate details by mistake)? e.g:
```
if(CXXThisExpr) {
details = {}
if (const) details += "const";
if (implicit) details += "implicit";
return join(",", details);
}
```
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:79
{R"cpp(
-template int root() {
- (void)root();
+namespace root {
+template int tmpl() {
is this change intentional ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91868/new/
https://reviews.llvm.org/D91868
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91868: [clangd] Mention when CXXThis is implicit in exposed AST.
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Seeing an implicit this in the AST is pretty confusing I think.
While here, also mention when `this` is const.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91868
Files:
clang-tools-extra/clangd/DumpAST.cpp
clang-tools-extra/clangd/unittests/DumpASTTests.cpp
Index: clang-tools-extra/clangd/unittests/DumpASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/DumpASTTests.cpp
+++ clang-tools-extra/clangd/unittests/DumpASTTests.cpp
@@ -76,29 +76,32 @@
type: Record - S
)"},
{R"cpp(
-template int root() {
- (void)root();
+namespace root {
+template int tmpl() {
+ (void)tmpl();
return T::value;
+}
}
)cpp",
R"(
-declaration: FunctionTemplate - root
- declaration: TemplateTypeParm - T
- declaration: Function - root
-type: FunctionProto
- type: Builtin - int
-statement: Compound
- expression: CStyleCast - ToVoid
-type: Builtin - void
-expression: Call
- expression: ImplicitCast - FunctionToPointerDecay
-expression: DeclRef - root
- template argument: Type
-type: Builtin - unsigned int
- statement: Return
-expression: DependentScopeDeclRef - value
- specifier: TypeSpec
-type: TemplateTypeParm - T
+declaration: Namespace - root
+ declaration: FunctionTemplate - tmpl
+declaration: TemplateTypeParm - T
+declaration: Function - tmpl
+ type: FunctionProto
+type: Builtin - int
+ statement: Compound
+expression: CStyleCast - ToVoid
+ type: Builtin - void
+ expression: Call
+expression: ImplicitCast - FunctionToPointerDecay
+ expression: DeclRef - tmpl
+template argument: Type
+ type: Builtin - unsigned int
+statement: Return
+ expression: DependentScopeDeclRef - value
+specifier: TypeSpec
+ type: TemplateTypeParm - T
)"},
{R"cpp(
struct Foo { char operator+(int); };
@@ -116,10 +119,28 @@
type: Record - Foo
expression: IntegerLiteral - 42
)"},
+ {R"cpp(
+struct Bar {
+ int x;
+ int root() const {
+return x;
+ }
+};
+ )cpp",
+ R"(
+declaration: CXXMethod - root
+ type: FunctionProto
+type: Builtin - int
+ statement: Compound
+statement: Return
+ expression: ImplicitCast - LValueToRValue
+expression: Member - x
+ expression: CXXThis - const, implicit
+ )"},
};
for (const auto &Case : Cases) {
ParsedAST AST = TestTU::withCode(Case.first).build();
-auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "root")),
+auto Node = dumpAST(DynTypedNode::create(findUnqualifiedDecl(AST, "root")),
AST.getTokens(), AST.getASTContext());
EXPECT_EQ(llvm::StringRef(Case.second).trim(),
llvm::StringRef(llvm::to_string(Node)).trim());
Index: clang-tools-extra/clangd/DumpAST.cpp
===
--- clang-tools-extra/clangd/DumpAST.cpp
+++ clang-tools-extra/clangd/DumpAST.cpp
@@ -230,6 +230,13 @@
return UnaryOperator::getOpcodeStr(UO->getOpcode()).str();
if (const auto *CCO = dyn_cast(S))
return CCO->getConstructor()->getNameAsString();
+if (const auto *CTE = dyn_cast(S)) {
+ bool Const = CTE->getType()->getPointeeType().isLocalConstQualified();
+ if (CTE->isImplicit())
+return Const ? "const, implicit" : "implicit";
+ if (Const)
+return "const";
+}
if (isa(S) || isa(S) ||
isa(S) || isa(S) ||
isa(S) || isa(S))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
