fix usr rhs #68329
fix usr rhs #68329
Conversation
You can test this locally with the following command:git-clang-format --diff 48a53509e851c93f352c967da1feb1c8fb2abd9a 80fa52f6efa36e04bf87c525013513d4429788ae -- clang/test/Index/USR/using-decl.cpp clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp clang/lib/Index/USRGeneration.cpp clang/test/Index/using_if_exists.cpp clang/test/Index/usrs.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp b/clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp index 64c8834ab..c8e5c1cf9 100644 --- a/clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp +++ b/clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp @@ -202,8 +202,8 @@ TEST(SymbolInfoTests, All) { "def_bool"}, ExpectedSymbolDetails{"foo", "", "c:@F@foo#I#", "def_int", "def_int"}, - ExpectedSymbolDetails{"foo", "bar::", "c:TestTU.cpp@N@bar@UD@foo", - "decl"}}}, + ExpectedSymbolDetails{ + "foo", "bar::", "c:TestTU.cpp@N@bar@UD@foo", "decl"}}}, { R"cpp( // Multiple symbols returned - implicit conversion struct foo {}; |
Make sure we include filename and namespace for the target declarations.
sam-mccall left a comment
sam-mccall left a comment There was a problem hiding this comment.
Thanks! This is a bit subtle, so I'm being a bit picky...
Can you add a more useful commit message?
| VisitDeclContext(D->getDeclContext()); | ||
| Out << "@UD"; | ||
| | ||
| // When the using-decl is resolved also print the context of the first target |
There was a problem hiding this comment.
what does "when the using-decl is resolved" mean?
I guess "when" just means "if", but UnresolvedUsing{Value,Typename}Decl aren't BaseUsingDecls, so what kind of "unresolved" are we talking about?
If it's just broken code, I think the comment isn't needed, the intent to defensively not-crash is clear.
| VisitDeclContext(D->getDeclContext()); | ||
| Out << "@UD"; | ||
| | ||
| // When the using-decl is resolved also print the context of the first target |
There was a problem hiding this comment.
again, say why rather than what:
We may have both using ns1::foo and using ns2::foo bringing in different overloads. Consider them different symbols.
| | ||
| void USRGenerator::VisitBaseUsingDecl(const BaseUsingDecl *D) { | ||
| // Add the filename when needed to disambiguate using decls from different | ||
| // files. |
There was a problem hiding this comment.
that much is clear from the code, it would be more useful to say why
our ideas about when using decls are the same vs different are dissimilar to other decls
| void USRGenerator::VisitBaseUsingDecl(const BaseUsingDecl *D) { | ||
| // Add the filename when needed to disambiguate using decls from different | ||
| // files. | ||
| if (ShouldGenerateLocation(D) && GenLoc(D, /*IncludeOffset=*/false)) |
There was a problem hiding this comment.
as discussed offline, ShouldGenerateLocation doesn't seem like the right check here:
- we definitely want to include the filename for *.cpp
- we may want to include the filename for *.h
- we probably should exclude system headers
- for using decls inside functions, we need to include the filename if the function is not externally visible (though it's probably fine to include it always)
3 participants
No description provided.