[Sema]Use tag name lookup for class names #112166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Gábor Spaits (spaits) ChangesThis PR would fix #16855 . I think the correct lookup to use for class names is Tag name lookup, because it does not take namespaces into account. The current lookup does and because of this some valid programs are not accepted. If you think that Tag name lookup is not correct for all cases when we are looking up types based on class names then we can only do tag name lookup when looking up class names for inheritance. In case of inheritance: Full diff: https://github.com/llvm/llvm-project/pull/112166.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 118873bc93ad4b..5cce92ef118951 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -357,9 +357,10 @@ ParsedType Sema::getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
return nullptr;
}
- // FIXME: LookupNestedNameSpecifierName isn't the right kind of
- // lookup for class-names.
- LookupNameKind Kind = isClassName ? LookupNestedNameSpecifierName :
+ // In case we know that the identifier is a class name, we know that it is
+ // a type declaration (struct, class, union or enum) so we can use tag name
+ // lookup.
+ LookupNameKind Kind = isClassName ? LookupTagName :
LookupOrdinaryName;
LookupResult Result(*this, &II, NameLoc, Kind);
if (LookupCtx) {
@@ -547,6 +548,7 @@ ParsedType Sema::getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
if (T.isNull()) {
// If it's not plausibly a type, suppress diagnostics.
Result.suppressDiagnostics();
+ llvm::errs() << "Not a type\n";
return nullptr;
}
diff --git a/clang/test/CXX/class.derived/p2.cpp b/clang/test/CXX/class.derived/p2.cpp
index 87e0f748615456..588436c3a8d211 100644
--- a/clang/test/CXX/class.derived/p2.cpp
+++ b/clang/test/CXX/class.derived/p2.cpp
@@ -7,3 +7,15 @@ namespace PR5840 {
int Base = 10;
struct Derived : Base {};
}
+
+namespace issue_number {
+ struct x {};
+ namespace
+ {
+ namespace x
+ {
+ struct y : x
+ {};
+ }
+ }
+}
\ No newline at end of file
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
spaits
changed the title
Sirraide
reviewed
Oct 14, 2024
|
Thank you @Sirraide for reviewing my PR. I have added this change to the release notes and addressed the other comments. |
Sirraide
reviewed
Oct 14, 2024
|
Thank you @Sirraide for reviewing. I will wait a few hours if other also want to take a look at this, then I will squash the commits and merge the changes. |
spaits
force-pushed
the
ignore-non-type-names
branch
2 times, most recently
from
64aa2ad to
6935e90
Compare
This PR would fix llvm#16855 . I think the correct lookup to use for class names is Tag name lookup, because it does not take namespaces into account. The current lookup does and because of this some valid programs are not accepted. If you think that Tag name lookup is not correct for all cases when we are looking up types based on class names then we can only do tag name lookup when looking up class names for inheritance. In case of inheritance: ``` [class.derived]p2 says: "During the lookup for a base class name, non-type names are ignored." ```
spaits
force-pushed
the
ignore-non-type-names
branch
from
6935e90 to
4afa9c5
Compare
|
The windows CI passes. The linux CI fails, but it also fails on other clang fronted PRs (see #112289 for example). I think this failure is unrelated to this PR. I will merge this. |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
This PR would fix llvm#16855 . The correct lookup to use for class names is Tag name lookup, because it does not take namespaces into account. The lookup before does and because of this some valid programs are not accepted. An example scenario of a valid program being declined is when you have a struct (let's call it `y`) inheriting from another struct with a name `x` but the struct `y` is in a namespace that is also called `x`: ``` struct x {}; namespace { namespace x { struct y : x {}; } } ``` This shall be accepted because: ``` C++ [class.derived]p2 (wrt lookup in a base-specifier): The lookup for // the component name of the type-name or simple-template-id is type-only. ```
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 17, 2024
This PR would fix llvm#16855 . The correct lookup to use for class names is Tag name lookup, because it does not take namespaces into account. The lookup before does and because of this some valid programs are not accepted. An example scenario of a valid program being declined is when you have a struct (let's call it `y`) inheriting from another struct with a name `x` but the struct `y` is in a namespace that is also called `x`: ``` struct x {}; namespace { namespace x { struct y : x {}; } } ``` This shall be accepted because: ``` C++ [class.derived]p2 (wrt lookup in a base-specifier): The lookup for // the component name of the type-name or simple-template-id is type-only. ```
EricWF
pushed a commit
to efcs/llvm-project
that referenced
this pull request
Oct 22, 2024
This PR would fix llvm#16855 . The correct lookup to use for class names is Tag name lookup, because it does not take namespaces into account. The lookup before does and because of this some valid programs are not accepted. An example scenario of a valid program being declined is when you have a struct (let's call it `y`) inheriting from another struct with a name `x` but the struct `y` is in a namespace that is also called `x`: ``` struct x {}; namespace { namespace x { struct y : x {}; } } ``` This shall be accepted because: ``` C++ [class.derived]p2 (wrt lookup in a base-specifier): The lookup for // the component name of the type-name or simple-template-id is type-only. ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR would fix #16855 .
I think the correct lookup to use for class names is Tag name lookup, because it does not take namespaces into account. The current lookup does and because of this some valid programs are not accepted.
If you think that Tag name lookup is not correct for all cases when we are looking up types based on class names then we can only do tag name lookup when looking up class names for inheritance. In case of inheritance: