[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 2 inline comments as done.
Closed by commit rL368287: [clangd] Added an early return from VisitMemberExpr
in SemanticHighlighting if… (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D65928?vs=214117=214122#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65928/new/
https://reviews.llvm.org/D65928
Files:
clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
// When calling the destructor manually like: AAA::~A(); The ~ is a
// MemberExpr. Other methods should still be highlighted though.
return true;
+if (isa(MD))
+ // The MemberLoc is invalid for C++ conversion operators. We do not
+ // attempt to add tokens with invalid locations.
+ return true;
addToken(ME->getMemberLoc(), MD);
return true;
}
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,23 @@
struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
extern template struct $Class[[Tmpl]];
template struct $Class[[Tmpl]];
+)cpp",
+// This test is to guard against highlightings disappearing when using
+// conversion operators as their behaviour in the clang AST differ from
+// other CXXMethodDecls.
+R"cpp(
+ class $Class[[Foo]] {};
+ struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+ };
+ void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+ }
)cpp"};
for (const auto : TestCases) {
checkHighlightings(TestCase);
Index: clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
// When calling the destructor manually like: AAA::~A(); The ~ is a
// MemberExpr. Other methods should still be highlighted though.
return true;
+if (isa(MD))
+ // The MemberLoc is invalid for C++ conversion operators. We do not
+ // attempt to add tokens with invalid locations.
+ return true;
addToken(ME->getMemberLoc(), MD);
return true;
}
Index: clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,23 @@
struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
extern template struct $Class[[Tmpl]];
template struct $Class[[Tmpl]];
+)cpp",
+// This test is to guard against highlightings disappearing when using
+// conversion operators as their behaviour in the clang AST differ from
+// other CXXMethodDecls.
+R"cpp(
+ class $Class[[Foo]] {};
+ struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+ };
+ void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+ }
)cpp"};
for (const auto : TestCases) {
checkHighlightings(TestCase);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
hokein added a comment. please update the patch description. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65928/new/ https://reviews.llvm.org/D65928 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56
+if (isa(MD))
+ // If the member is a conversion operator the loc will be invalid. This
+ // causes the addToken function to emit a lot of error logs about trying
jvikstrom wrote:
> hokein wrote:
> > Maybe just `The MemberLoc is invalid for C++ conversion operato , we don't
> > attempt to add a token for an invalid location`?
> >
> >
> > Does the location is always invalid? or just for builtin types? e.g.
> > ```
> > class Foo {};
> > struct Bar {
> > explicit operator Foo*() const; // 1
> > explicit operator int() const; // 2
> > };
> > ```
> Builtin types has nothing to do with it. It's invalid for every conversion
> operator. Will actually add a test just to make sure that everything else is
> still highlighted correctly.
ah, I thought it is the operator declaration, but here we mean expression.
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:268
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
nit: could you add a comment describing the purpose of this test?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65928/new/
https://reviews.llvm.org/D65928
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
jvikstrom marked an inline comment as done.
jvikstrom added inline comments.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56
+if (isa(MD))
+ // If the member is a conversion operator the loc will be invalid. This
+ // causes the addToken function to emit a lot of error logs about trying
hokein wrote:
> Maybe just `The MemberLoc is invalid for C++ conversion operato , we don't
> attempt to add a token for an invalid location`?
>
>
> Does the location is always invalid? or just for builtin types? e.g.
> ```
> class Foo {};
> struct Bar {
> explicit operator Foo*() const; // 1
> explicit operator int() const; // 2
> };
> ```
Builtin types has nothing to do with it. It's invalid for every conversion
operator. Will actually add a test just to make sure that everything else is
still highlighted correctly.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65928/new/
https://reviews.llvm.org/D65928
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
jvikstrom updated this revision to Diff 214117.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.
Added test for conversion operators. Also changed comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65928/new/
https://reviews.llvm.org/D65928
Files:
clang-tools-extra/clangd/SemanticHighlighting.cpp
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,20 @@
struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
extern template struct $Class[[Tmpl]];
template struct $Class[[Tmpl]];
+)cpp",
+R"cpp(
+ class $Class[[Foo]] {};
+ struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+ };
+ void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+ }
)cpp"};
for (const auto : TestCases) {
checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
// When calling the destructor manually like: AAA::~A(); The ~ is a
// MemberExpr. Other methods should still be highlighted though.
return true;
+if (isa(MD))
+ // The MemberLoc is invalid for C++ conversion operators. We do not
+ // attempt to add tokens with invalid locations.
+ return true;
addToken(ME->getMemberLoc(), MD);
return true;
}
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -255,6 +255,20 @@
struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
extern template struct $Class[[Tmpl]];
template struct $Class[[Tmpl]];
+)cpp",
+R"cpp(
+ class $Class[[Foo]] {};
+ struct $Class[[Bar]] {
+explicit operator $Class[[Foo]]*() const;
+explicit operator int() const;
+operator $Class[[Foo]]();
+ };
+ void $Function[[f]]() {
+$Class[[Bar]] $Variable[[B]];
+$Class[[Foo]] $Variable[[F]] = $Variable[[B]];
+$Class[[Foo]] *$Variable[[FP]] = ($Class[[Foo]]*)$Variable[[B]];
+int $Variable[[I]] = (int)$Variable[[B]];
+ }
)cpp"};
for (const auto : TestCases) {
checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -52,6 +52,10 @@
// When calling the destructor manually like: AAA::~A(); The ~ is a
// MemberExpr. Other methods should still be highlighted though.
return true;
+if (isa(MD))
+ // The MemberLoc is invalid for C++ conversion operators. We do not
+ // attempt to add tokens with invalid locations.
+ return true;
addToken(ME->getMemberLoc(), MD);
return true;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
hokein added inline comments.
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56
+if (isa(MD))
+ // If the member is a conversion operator the loc will be invalid. This
+ // causes the addToken function to emit a lot of error logs about trying
Maybe just `The MemberLoc is invalid for C++ conversion operato , we don't
attempt to add a token for an invalid location`?
Does the location is always invalid? or just for builtin types? e.g.
```
class Foo {};
struct Bar {
explicit operator Foo*() const; // 1
explicit operator int() const; // 2
};
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65928/new/
https://reviews.llvm.org/D65928
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
jvikstrom updated this revision to Diff 214110. jvikstrom marked an inline comment as done. jvikstrom added a comment. Changed to do an isa check instead of checking if MemberLoc is invalid. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65928/new/ https://reviews.llvm.org/D65928 Files: clang-tools-extra/clangd/SemanticHighlighting.cpp Index: clang-tools-extra/clangd/SemanticHighlighting.cpp === --- clang-tools-extra/clangd/SemanticHighlighting.cpp +++ clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -52,6 +52,11 @@ // When calling the destructor manually like: AAA::~A(); The ~ is a // MemberExpr. Other methods should still be highlighted though. return true; +if (isa(MD)) + // If the member is a conversion operator the loc will be invalid. This + // causes the addToken function to emit a lot of error logs about trying + // to add a token with an invalid range. + return true; addToken(ME->getMemberLoc(), MD); return true; } Index: clang-tools-extra/clangd/SemanticHighlighting.cpp === --- clang-tools-extra/clangd/SemanticHighlighting.cpp +++ clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -52,6 +52,11 @@ // When calling the destructor manually like: AAA::~A(); The ~ is a // MemberExpr. Other methods should still be highlighted though. return true; +if (isa(MD)) + // If the member is a conversion operator the loc will be invalid. This + // causes the addToken function to emit a lot of error logs about trying + // to add a token with an invalid range. + return true; addToken(ME->getMemberLoc(), MD); return true; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:56 +if (ME->getMemberLoc().isInvalid()) + // If the member is a conversion operator the loc will be invalid. This + // causes the addToken function to emit a lot of error logs about trying I think we only want to ignore the conversion operator, we could do an early return when MD is a cxx conversion decl, e.g. ` isa(MD)`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65928/new/ https://reviews.llvm.org/D65928 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65928: [clangd] Added an early return from VisitMemberExpr in SemanticHighlighting if the MemberLoc is invalid.
jvikstrom created this revision. jvikstrom added reviewers: hokein, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Conversion operators contain invalid MemberLocs which causes SemanticHighlighting to emit a lot of error logs in large files as they can occur fairly often (for example converting StringRef to std string). As the only thing happening was a lot of error logs being emited there doesn't really seem to be any way to test this (no erroneous tokens are added). But emiting as many logs as were being emited is not wanted. Can't really be patched elsewhere. RecursiveASTVisitor should still traverse the expr as it can contain other "non-implicit" decls/exprs that must be visited (for example DeclRefs). A potential fix could be to special case the TraverseMemberExpr function to not Walk* the Expr if it has an invalid MemberLoc. But that would change the behaviour of RecursiveASTVisitor to be unexpected. (and to change the behaviour to be this for all exprs would change the contract of RecursiveASTVisitor to much) Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D65928 Files: clang-tools-extra/clangd/SemanticHighlighting.cpp Index: clang-tools-extra/clangd/SemanticHighlighting.cpp === --- clang-tools-extra/clangd/SemanticHighlighting.cpp +++ clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -52,6 +52,11 @@ // When calling the destructor manually like: AAA::~A(); The ~ is a // MemberExpr. Other methods should still be highlighted though. return true; +if (ME->getMemberLoc().isInvalid()) + // If the member is a conversion operator the loc will be invalid. This + // causes the addToken function to emit a lot of error logs about trying + // to add a token with an invalid range. + return true; addToken(ME->getMemberLoc(), MD); return true; } Index: clang-tools-extra/clangd/SemanticHighlighting.cpp === --- clang-tools-extra/clangd/SemanticHighlighting.cpp +++ clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -52,6 +52,11 @@ // When calling the destructor manually like: AAA::~A(); The ~ is a // MemberExpr. Other methods should still be highlighted though. return true; +if (ME->getMemberLoc().isInvalid()) + // If the member is a conversion operator the loc will be invalid. This + // causes the addToken function to emit a lot of error logs about trying + // to add a token with an invalid range. + return true; addToken(ME->getMemberLoc(), MD); return true; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
