[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE327387: [clangd] Fix irrelevant declaratations in goto
definition (on macros). (authored by hokein, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44293?vs=138160=138162#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
class [[Foo]] {};
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -127,6 +127,16 @@
MacroInfo *MacroInf = MacroDef.getMacroInfo();
if (MacroInf) {
MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+ // Clear all collected delcarations if this is a macro search.
+ //
+ // In theory, there should be no declarataions being collected when
we
+ // search a source location that refers to a macro.
+ // The occurrence location returned by `handleDeclOccurence` is
+ // limited (FID, Offset are from expansion location), we will collect
+ // all declarations inside the macro.
+ //
+ // FIXME: Avoid adding decls from inside macros in
handlDeclOccurence.
+ Decls.clear();
}
}
}
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
class [[Foo]] {};
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -127,6 +127,16 @@
MacroInfo *MacroInf = MacroDef.getMacroInfo();
if (MacroInf) {
MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+ // Clear all collected delcarations if this is a macro search.
+ //
+ // In theory, there should be no declarataions being collected when we
+ // search a source location that refers to a macro.
+ // The occurrence location returned by `handleDeclOccurence` is
+ // limited (FID, Offset are from expansion location), we will collect
+ // all declarations inside the macro.
+ //
+ // FIXME: Avoid adding decls from inside macros in handlDeclOccurence.
+ Decls.clear();
}
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
hokein updated this revision to Diff 138160.
hokein marked an inline comment as done.
hokein added a comment.
Rephrase the fixme.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
class [[Foo]] {};
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -127,6 +127,16 @@
MacroInfo *MacroInf = MacroDef.getMacroInfo();
if (MacroInf) {
MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+ // Clear all collected delcarations if this is a macro search.
+ //
+ // In theory, there should be no declarataions being collected when
we
+ // search a source location that refers to a macro.
+ // The occurrence location returned by `handleDeclOccurence` is
+ // limited (FID, Offset are from expansion location), we will collect
+ // all declarations inside the macro.
+ //
+ // FIXME: Avoid adding decls from inside macros in
handlDeclOccurence.
+ Decls.clear();
}
}
}
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
class [[Foo]] {};
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -127,6 +127,16 @@
MacroInfo *MacroInf = MacroDef.getMacroInfo();
if (MacroInf) {
MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+ // Clear all collected delcarations if this is a macro search.
+ //
+ // In theory, there should be no declarataions being collected when we
+ // search a source location that refers to a macro.
+ // The occurrence location returned by `handleDeclOccurence` is
+ // limited (FID, Offset are from expansion location), we will collect
+ // all declarations inside the macro.
+ //
+ // FIXME: Avoid adding decls from inside macros in handlDeclOccurence.
+ Decls.clear();
}
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small nit about the comment. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44293 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
ilya-biryukov added inline comments. Comment at: clangd/XRefs.cpp:137 + // + // FIXME: Exclude declarations from macros. + Decls.clear(); NIT: the fixme was a bit hard to follow for me. Maybe make it more clear where the problem should be handled, e.g. ```FIXME: we should avoid adding decls from inside macros in handlDeclOccurence. ``` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44293 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
hokein added inline comments.
Comment at: clangd/XRefs.cpp:201
std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ if (!MacroInfos.empty()) {
+for (auto Item : MacroInfos) {
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > I wonder whether we should fix the `DeclrationAndMacrosFinder` to not
> > > return declarations coming from macro instantiations instead.
> > > There are other clients (e.g. document highlights) that will probably
> > > break in the same way.
> > >
> > > Do you think it would be possible and it would make sense for
> > > `DeclrationAndMacrosFinder` to only return a macro in this case and not
> > > return Decls coming from macro expansions?
> > I thought about it initially, but the information provided
> > `handleDeclOccurrence` is limited...the occurrence location (`FID` and
> > `Offset`) is expansion location (which is not always we want). That being
> > said, when GoToDefinition on a macro, all declarations inside the macro
> > body will be picked up.
> >
> > In document hover implementation, we also use the same mechanism to avoid
> > this problem :(
> >
> As discussed offline, we should probably move this logic to
> `DeclrationAndMacrosFinder` so that all its clients (hover,
> documentHighlights, findDefinitions) are consistent.
> Having a single function in `DeclrationAndMacrosFinder` that returns results
> (currently there are two: `takeDecls()` and `takeMacros()`) would allow that
> with minimal changes.
As discussed, made the workaround in `Finder` to keep the change minimal, no
changes are required in client side.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
hokein updated this revision to Diff 138147.
hokein marked an inline comment as done.
hokein added a comment.
Address review comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
[[class Foo {}]];
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -126,6 +126,16 @@
MacroInfo *MacroInf = MacroDef.getMacroInfo();
if (MacroInf) {
MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+ // Clear all collected delcarations if this is a macro search.
+ //
+ // In theory, there should be no declarataions being collected when
we
+ // search a source location that refers to a macro.
+ // The occurrence location returned by `handleDeclOccurence` is
+ // limited (FID, Offset are from expansion location), we will collect
+ // all declarations inside the macro.
+ //
+ // FIXME: Exclude declarations from macros.
+ Decls.clear();
}
}
}
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
[[class Foo {}]];
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -126,6 +126,16 @@
MacroInfo *MacroInf = MacroDef.getMacroInfo();
if (MacroInf) {
MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
+ // Clear all collected delcarations if this is a macro search.
+ //
+ // In theory, there should be no declarataions being collected when we
+ // search a source location that refers to a macro.
+ // The occurrence location returned by `handleDeclOccurence` is
+ // limited (FID, Offset are from expansion location), we will collect
+ // all declarations inside the macro.
+ //
+ // FIXME: Exclude declarations from macros.
+ Decls.clear();
}
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
ilya-biryukov added inline comments.
Comment at: clangd/XRefs.cpp:201
std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ if (!MacroInfos.empty()) {
+for (auto Item : MacroInfos) {
hokein wrote:
> ilya-biryukov wrote:
> > I wonder whether we should fix the `DeclrationAndMacrosFinder` to not
> > return declarations coming from macro instantiations instead.
> > There are other clients (e.g. document highlights) that will probably break
> > in the same way.
> >
> > Do you think it would be possible and it would make sense for
> > `DeclrationAndMacrosFinder` to only return a macro in this case and not
> > return Decls coming from macro expansions?
> I thought about it initially, but the information provided
> `handleDeclOccurrence` is limited...the occurrence location (`FID` and
> `Offset`) is expansion location (which is not always we want). That being
> said, when GoToDefinition on a macro, all declarations inside the macro body
> will be picked up.
>
> In document hover implementation, we also use the same mechanism to avoid
> this problem :(
>
As discussed offline, we should probably move this logic to
`DeclrationAndMacrosFinder` so that all its clients (hover, documentHighlights,
findDefinitions) are consistent.
Having a single function in `DeclrationAndMacrosFinder` that returns results
(currently there are two: `takeDecls()` and `takeMacros()`) would allow that
with minimal changes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
hokein added inline comments.
Comment at: clangd/XRefs.cpp:201
std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ if (!MacroInfos.empty()) {
+for (auto Item : MacroInfos) {
ilya-biryukov wrote:
> I wonder whether we should fix the `DeclrationAndMacrosFinder` to not return
> declarations coming from macro instantiations instead.
> There are other clients (e.g. document highlights) that will probably break
> in the same way.
>
> Do you think it would be possible and it would make sense for
> `DeclrationAndMacrosFinder` to only return a macro in this case and not
> return Decls coming from macro expansions?
I thought about it initially, but the information provided
`handleDeclOccurrence` is limited...the occurrence location (`FID` and
`Offset`) is expansion location (which is not always we want). That being said,
when GoToDefinition on a macro, all declarations inside the macro body will be
picked up.
In document hover implementation, we also use the same mechanism to avoid this
problem :(
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
ilya-biryukov added inline comments.
Comment at: clangd/XRefs.cpp:201
std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ if (!MacroInfos.empty()) {
+for (auto Item : MacroInfos) {
I wonder whether we should fix the `DeclrationAndMacrosFinder` to not return
declarations coming from macro instantiations instead.
There are other clients (e.g. document highlights) that will probably break in
the same way.
Do you think it would be possible and it would make sense for
`DeclrationAndMacrosFinder` to only return a macro in this case and not return
Decls coming from macro expansions?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44293: [clangd] Fix irrelevant declaratations in goto definition (on macros).
hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: ioeric, jkorous-apple, klimek.
DeclrationAndMacrosFinder will find some declarations (not macro!) that are
referened inside the macro somehow, isSearchedLocation() is not sufficient, we
don't know whether the searched source location is macro or not.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44293
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
[[class Foo {}]];
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -197,19 +197,21 @@
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
- std::vector Decls = DeclMacrosFinder->takeDecls();
std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ if (!MacroInfos.empty()) {
+for (auto Item : MacroInfos) {
+ SourceRange SR(Item.Info->getDefinitionLoc(),
+ Item.Info->getDefinitionEndLoc());
+ auto L = getDeclarationLocation(AST, SR);
+ if (L)
+Result.push_back(*L);
+}
- for (auto Item : Decls) {
-auto L = getDeclarationLocation(AST, Item->getSourceRange());
-if (L)
- Result.push_back(*L);
+return Result;
}
- for (auto Item : MacroInfos) {
-SourceRange SR(Item.Info->getDefinitionLoc(),
- Item.Info->getDefinitionEndLoc());
-auto L = getDeclarationLocation(AST, SR);
+ for (auto Item : DeclMacrosFinder->takeDecls()) {
+auto L = getDeclarationLocation(AST, Item->getSourceRange());
if (L)
Result.push_back(*L);
}
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -213,6 +213,15 @@
#undef macro
)cpp",
+ R"cpp(// Macro
+ class TTT { public: int a; };
+ #define [[FF(S) if (int b = S.a) {}]]
+ void f() {
+ TTT t;
+ F^F(t);
+ }
+ )cpp",
+
R"cpp(// Forward class declaration
class Foo;
[[class Foo {}]];
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -197,19 +197,21 @@
indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
DeclMacrosFinder, IndexOpts);
- std::vector Decls = DeclMacrosFinder->takeDecls();
std::vector MacroInfos = DeclMacrosFinder->takeMacroInfos();
+ if (!MacroInfos.empty()) {
+for (auto Item : MacroInfos) {
+ SourceRange SR(Item.Info->getDefinitionLoc(),
+ Item.Info->getDefinitionEndLoc());
+ auto L = getDeclarationLocation(AST, SR);
+ if (L)
+Result.push_back(*L);
+}
- for (auto Item : Decls) {
-auto L = getDeclarationLocation(AST, Item->getSourceRange());
-if (L)
- Result.push_back(*L);
+return Result;
}
- for (auto Item : MacroInfos) {
-SourceRange SR(Item.Info->getDefinitionLoc(),
- Item.Info->getDefinitionEndLoc());
-auto L = getDeclarationLocation(AST, SR);
+ for (auto Item : DeclMacrosFinder->takeDecls()) {
+auto L = getDeclarationLocation(AST, Item->getSourceRange());
if (L)
Result.push_back(*L);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
