[Clang] Implement resolution for CWG1835 #92957
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
|
This PR is by no means complete, but I would like feedback regarding the validity of my interpretation of the DR resolution and perhaps some guidance with how to best go about implementing it. The currently included implementation works, but I'm uncertain whether it's the best way of going about it. |
|
It was a pure accident I opened this PR. I think draft status does a good job at silencing notifications. |
|
@Endilll I'm still in the process of writing tests, but I'll add several soon. I'm quite confident about my interpretation being correct but I want to be 100% sure :). At the very least this patch gets [basic.lookup.qual.general] example 3 right, in addition to the examples I provided in the PR. |
mizvekov
reviewed
May 22, 2024
Comment on lines
50
to
51
| this->Derived1T<T>::Foo(); // expected-error{{use 'template' keyword to treat 'Derived1T' as a dependent template name}} | ||
| this->Derived2T<T>::Member = 42; // expected-error{{use 'template' keyword to treat 'Derived2T' as a dependent template name}} |
There was a problem hiding this comment.
This is an access control test:
Suggested change
| this->Derived1T<T>::Foo(); // expected-error{{use 'template' keyword to treat 'Derived1T' as a dependent template name}} | |
| this->Derived2T<T>::Member = 42; // expected-error{{use 'template' keyword to treat 'Derived2T' as a dependent template name}} | |
| this->template Derived1T<T>::Foo(); | |
| this->template Derived2T<T>::Member = 42; |
clang/lib/Sema/SemaCXXScopeSpec.cpp
Outdated
Comment on lines
400
to
381
| // FIXME: This is a rather nasty hack! Ideally we should get the results | ||
| // from LookupTemplateName/BuildCXXNestedNameSpecifier. | ||
| const IdentifierInfo *II = NNS->getAsIdentifier(); | ||
| if (!II) { | ||
| if (const auto *DTST = | ||
| dyn_cast_if_present<DependentTemplateSpecializationType>( | ||
| NNS->getAsType())) | ||
| II = DTST->getIdentifier(); | ||
| else | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
I am not sure why you think this is a nasty hack, this looks legitimate to me.
I think this looks ugly because of the lack of a NNS kind for an Identifier with template arguments, so we abuse a type kind storing a DependentTemplateSpecializationType with no qualifier.
clang/test/CXX/basic/basic.lookup/basic.lookup.classref/p1-cxx11.cpp
Outdated
Show resolved
Hide resolved
sdkrystian
force-pushed
the
cwg-1835-rev-3
branch
from
574fff4
to
3bb8802
Compare
|
I added support for unqualified lookup finding multiple declarations in the most recent commits, fixing the crash the currently happens on clang assertions trunk for the following (godbolt link): inline namespace N
{
struct A { };
}
struct A { };
template<typename T>
void f(T& t)
{
t.A::x; // crash here
} |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-coroutines Author: Krystian Stasiowski (sdkrystian) ChangesCWG1835 was one of the many core issues resolved by P1787R6: Declarations and where to find them. Its resolution changes how [member-qualified names](http://eel.is/c++draft/basic.lookup.qual.general#2) are looked up. This patch is a draft implementation of that resolution. Previously, an identifier following The new wording in [[basic.lookup.qual.general] p2](eel.is/c++draft/basic.lookup.qual.general#2) states: And [[basic.lookup.qual.general] p3](eel.is/c++draft/basic.lookup.qual.general#3) now states: In non-standardese terms, these two paragraphs essentially state the following:
template<typename T>
struct A
{
void f(T* t)
{
this->x; // type of the object expression is 'A<T>'. although 'A<T>' is dependent, it is the
// current instantiation so we look up 'x' in the template definition context.
t->y; // type of the object expression is 'T' ('->' is transformed to '.' per [expr.ref]).
// 'T' is dependent and is *not* the current instantiation, so we lookup 'y' in the
// template instantiation context.
}
};
Although the second (unqualified) lookup is stated not to occur when the member-qualified name is dependent, a dependent name will not be dependent once the template is instantiated, so the second lookup must "occur" during instantiation if qualified lookup does not find anything. This means that we must perform the second (unqualified) lookup during parsing even when the type of the object expression is dependent, but those results are not used to determine whether a In even simpler terms (paraphrasing the meeting minutes from the review of P1787):
An example: struct A
{
void f();
};
template<typename T>
using B = A;
template<typename T>
struct C : A
{
template<typename U>
void g();
void h(T* t)
{
this->g<int>(); // ok, '<' is the start of a template-argument-list ('g' was found via qualified lookup in the current instantiation)
this->B<void>::f(); // ok, '<' is the start of a template-argument-list (current instantiation has no dependent bases, 'B' was found via unqualified lookup)
t->g<int>(); // error: '<' means less than (unqualified lookup does not occur for a member-qualified name that isn't the first component of a nested-name-specifier)
t->B<void>::f(); // error: '<' means less than (unqualified lookup does not occur if the name is dependent)
t->template B<void>::f(); // ok: '<' is the start of a template-argument-list ('template' keyword used)
}
};Some additional notes:
Patch is 113.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92957.diff 34 Files Affected:
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index dbf693611a7fa..6cd6e049ad1e9 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -3671,9 +3671,9 @@ class CXXUnresolvedConstructExpr final
/// an implicit access if a qualifier is provided.
class CXXDependentScopeMemberExpr final
: public Expr,
- private llvm::TrailingObjects<CXXDependentScopeMemberExpr,
- ASTTemplateKWAndArgsInfo,
- TemplateArgumentLoc, NamedDecl *> {
+ private llvm::TrailingObjects<
+ CXXDependentScopeMemberExpr, NestedNameSpecifierLoc, DeclAccessPair,
+ ASTTemplateKWAndArgsInfo, TemplateArgumentLoc> {
friend class ASTStmtReader;
friend class ASTStmtWriter;
friend TrailingObjects;
@@ -3686,17 +3686,15 @@ class CXXDependentScopeMemberExpr final
/// implicit accesses.
QualType BaseType;
- /// The nested-name-specifier that precedes the member name, if any.
- /// FIXME: This could be in principle store as a trailing object.
- /// However the performance impact of doing so should be investigated first.
- NestedNameSpecifierLoc QualifierLoc;
-
/// The member to which this member expression refers, which
/// can be name, overloaded operator, or destructor.
///
/// FIXME: could also be a template-id
DeclarationNameInfo MemberNameInfo;
+ /// The location of the '->' or '.' operator.
+ SourceLocation OperatorLoc;
+
// CXXDependentScopeMemberExpr is followed by several trailing objects,
// some of which optional. They are in order:
//
@@ -3716,8 +3714,16 @@ class CXXDependentScopeMemberExpr final
return CXXDependentScopeMemberExprBits.HasTemplateKWAndArgsInfo;
}
- bool hasFirstQualifierFoundInScope() const {
- return CXXDependentScopeMemberExprBits.HasFirstQualifierFoundInScope;
+ unsigned getNumUnqualifiedLookups() const {
+ return CXXDependentScopeMemberExprBits.NumUnqualifiedLookups;
+ }
+
+ unsigned numTrailingObjects(OverloadToken<NestedNameSpecifierLoc>) const {
+ return hasQualifier();
+ }
+
+ unsigned numTrailingObjects(OverloadToken<DeclAccessPair>) const {
+ return getNumUnqualifiedLookups();
}
unsigned numTrailingObjects(OverloadToken<ASTTemplateKWAndArgsInfo>) const {
@@ -3728,33 +3734,32 @@ class CXXDependentScopeMemberExpr final
return getNumTemplateArgs();
}
- unsigned numTrailingObjects(OverloadToken<NamedDecl *>) const {
- return hasFirstQualifierFoundInScope();
- }
-
CXXDependentScopeMemberExpr(const ASTContext &Ctx, Expr *Base,
QualType BaseType, bool IsArrow,
SourceLocation OperatorLoc,
NestedNameSpecifierLoc QualifierLoc,
SourceLocation TemplateKWLoc,
- NamedDecl *FirstQualifierFoundInScope,
+ ArrayRef<DeclAccessPair> UnqualifiedLookups,
DeclarationNameInfo MemberNameInfo,
const TemplateArgumentListInfo *TemplateArgs);
- CXXDependentScopeMemberExpr(EmptyShell Empty, bool HasTemplateKWAndArgsInfo,
- bool HasFirstQualifierFoundInScope);
+ CXXDependentScopeMemberExpr(EmptyShell Empty, bool HasQualifier,
+ unsigned NumUnqualifiedLookups,
+ bool HasTemplateKWAndArgsInfo);
public:
static CXXDependentScopeMemberExpr *
Create(const ASTContext &Ctx, Expr *Base, QualType BaseType, bool IsArrow,
SourceLocation OperatorLoc, NestedNameSpecifierLoc QualifierLoc,
- SourceLocation TemplateKWLoc, NamedDecl *FirstQualifierFoundInScope,
+ SourceLocation TemplateKWLoc,
+ ArrayRef<DeclAccessPair> UnqualifiedLookups,
DeclarationNameInfo MemberNameInfo,
const TemplateArgumentListInfo *TemplateArgs);
static CXXDependentScopeMemberExpr *
- CreateEmpty(const ASTContext &Ctx, bool HasTemplateKWAndArgsInfo,
- unsigned NumTemplateArgs, bool HasFirstQualifierFoundInScope);
+ CreateEmpty(const ASTContext &Ctx, bool HasQualifier,
+ unsigned NumUnqualifiedLookups, bool HasTemplateKWAndArgsInfo,
+ unsigned NumTemplateArgs);
/// True if this is an implicit access, i.e. one in which the
/// member being accessed was not written in the source. The source
@@ -3779,34 +3784,35 @@ class CXXDependentScopeMemberExpr final
bool isArrow() const { return CXXDependentScopeMemberExprBits.IsArrow; }
/// Retrieve the location of the '->' or '.' operator.
- SourceLocation getOperatorLoc() const {
- return CXXDependentScopeMemberExprBits.OperatorLoc;
+ SourceLocation getOperatorLoc() const { return OperatorLoc; }
+
+ /// Determines whether this member expression had a nested-name-specifier
+ /// prior to the name of the member, e.g., x->Base::foo.
+ bool hasQualifier() const {
+ return CXXDependentScopeMemberExprBits.HasQualifier;
}
- /// Retrieve the nested-name-specifier that qualifies the member name.
- NestedNameSpecifier *getQualifier() const {
- return QualifierLoc.getNestedNameSpecifier();
+ /// If the member name was qualified, retrieves the nested-name-specifier
+ /// that precedes the member name, with source-location information.
+ NestedNameSpecifierLoc getQualifierLoc() const {
+ if (!hasQualifier())
+ return NestedNameSpecifierLoc();
+ return *getTrailingObjects<NestedNameSpecifierLoc>();
}
- /// Retrieve the nested-name-specifier that qualifies the member
- /// name, with source location information.
- NestedNameSpecifierLoc getQualifierLoc() const { return QualifierLoc; }
+ /// If the member name was qualified, retrieves the
+ /// nested-name-specifier that precedes the member name. Otherwise, returns
+ /// NULL.
+ NestedNameSpecifier *getQualifier() const {
+ return getQualifierLoc().getNestedNameSpecifier();
+ }
- /// Retrieve the first part of the nested-name-specifier that was
- /// found in the scope of the member access expression when the member access
- /// was initially parsed.
- ///
- /// This function only returns a useful result when member access expression
- /// uses a qualified member name, e.g., "x.Base::f". Here, the declaration
- /// returned by this function describes what was found by unqualified name
- /// lookup for the identifier "Base" within the scope of the member access
- /// expression itself. At template instantiation time, this information is
- /// combined with the results of name lookup into the type of the object
- /// expression itself (the class type of x).
- NamedDecl *getFirstQualifierFoundInScope() const {
- if (!hasFirstQualifierFoundInScope())
- return nullptr;
- return *getTrailingObjects<NamedDecl *>();
+ /// Retrieve the declarations found by unqualified lookup for the first
+ /// component name of the nested-name-specifier, if any.
+ ArrayRef<DeclAccessPair> unqualified_lookups() const {
+ if (!getNumUnqualifiedLookups())
+ return std::nullopt;
+ return {getTrailingObjects<DeclAccessPair>(), getNumUnqualifiedLookups()};
}
/// Retrieve the name of the member that this expression refers to.
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 9cd7a364cd3f1..257a61c97c9c6 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1020,18 +1020,19 @@ class alignas(void *) Stmt {
LLVM_PREFERRED_TYPE(bool)
unsigned IsArrow : 1;
+ /// True if this member expression used a nested-name-specifier to
+ /// refer to the member, e.g., "x->Base::f".
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned HasQualifier : 1;
+
/// Whether this member expression has info for explicit template
/// keyword and arguments.
LLVM_PREFERRED_TYPE(bool)
unsigned HasTemplateKWAndArgsInfo : 1;
- /// See getFirstQualifierFoundInScope() and the comment listing
- /// the trailing objects.
- LLVM_PREFERRED_TYPE(bool)
- unsigned HasFirstQualifierFoundInScope : 1;
-
- /// The location of the '->' or '.' operator.
- SourceLocation OperatorLoc;
+ /// Number of declarations found by unqualified lookup for the
+ /// first component name of the nested-name-specifier.
+ unsigned NumUnqualifiedLookups;
};
class OverloadExprBitfields {
diff --git a/clang/include/clang/AST/UnresolvedSet.h b/clang/include/clang/AST/UnresolvedSet.h
index ee31be969b6e3..e2083e2a0bd24 100644
--- a/clang/include/clang/AST/UnresolvedSet.h
+++ b/clang/include/clang/AST/UnresolvedSet.h
@@ -96,6 +96,10 @@ class UnresolvedSetImpl {
decls().push_back(DeclAccessPair::make(D, AS));
}
+ void addAllDecls(ArrayRef<DeclAccessPair> Other) {
+ append(iterator(Other.begin()), iterator(Other.end()));
+ }
+
/// Replaces the given declaration with the new one, once.
///
/// \return true if the set changed
diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 23bc780e04979..231f3fc6fec83 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -75,6 +75,7 @@ class CXXScopeSpec {
SourceRange Range;
NestedNameSpecifierLocBuilder Builder;
ArrayRef<TemplateParameterList *> TemplateParamLists;
+ ArrayRef<DeclAccessPair> UnqualifiedLookups;
public:
SourceRange getRange() const { return Range; }
@@ -91,6 +92,13 @@ class CXXScopeSpec {
return TemplateParamLists;
}
+ void setUnqualifiedLookups(ArrayRef<DeclAccessPair> Found) {
+ UnqualifiedLookups = Found;
+ }
+ ArrayRef<DeclAccessPair> getUnqualifiedLookups() const {
+ return UnqualifiedLookups;
+ }
+
/// Retrieve the representation of the nested-name-specifier.
NestedNameSpecifier *getScopeRep() const {
return Builder.getRepresentation();
diff --git a/clang/include/clang/Sema/Lookup.h b/clang/include/clang/Sema/Lookup.h
index b0a08a05ac6a0..6b765ef3c980f 100644
--- a/clang/include/clang/Sema/Lookup.h
+++ b/clang/include/clang/Sema/Lookup.h
@@ -483,11 +483,15 @@ class LookupResult {
ResultKind = Found;
}
+ void addAllDecls(ArrayRef<DeclAccessPair> Other) {
+ Decls.addAllDecls(Other);
+ ResultKind = Found;
+ }
+
/// Add all the declarations from another set of lookup
/// results.
void addAllDecls(const LookupResult &Other) {
- Decls.append(Other.Decls.begin(), Other.Decls.end());
- ResultKind = Found;
+ addAllDecls(Other.Decls.pairs());
}
/// Determine whether no result was found because we could not
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index e6296868000c5..5628d961c853a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2380,7 +2380,8 @@ class Sema final : public SemaBase {
bool isAcceptableNestedNameSpecifier(const NamedDecl *SD,
bool *CanCorrect = nullptr);
- NamedDecl *FindFirstQualifierInScope(Scope *S, NestedNameSpecifier *NNS);
+ bool LookupFirstQualifierInScope(Scope *S, NestedNameSpecifier *NNS,
+ UnresolvedSetImpl &R);
/// Keeps information about an identifier in a nested-name-spec.
///
@@ -2413,7 +2414,6 @@ class Sema final : public SemaBase {
bool BuildCXXNestedNameSpecifier(Scope *S, NestedNameSpecInfo &IdInfo,
bool EnteringContext, CXXScopeSpec &SS,
- NamedDecl *ScopeLookupResult,
bool ErrorRecoveryLookup,
bool *IsCorrectedToColon = nullptr,
bool OnlyNamespace = false);
@@ -6913,14 +6913,15 @@ class Sema final : public SemaBase {
const TemplateArgumentListInfo *TemplateArgs,
bool IsDefiniteInstance, const Scope *S);
- ExprResult ActOnDependentMemberExpr(
- Expr *Base, QualType BaseType, bool IsArrow, SourceLocation OpLoc,
- const CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
- NamedDecl *FirstQualifierInScope, const DeclarationNameInfo &NameInfo,
- const TemplateArgumentListInfo *TemplateArgs);
+ ExprResult
+ ActOnDependentMemberExpr(Expr *Base, QualType BaseType, bool IsArrow,
+ SourceLocation OpLoc, const CXXScopeSpec &SS,
+ SourceLocation TemplateKWLoc,
+ const DeclarationNameInfo &NameInfo,
+ const TemplateArgumentListInfo *TemplateArgs);
ExprResult ActOnMemberAccessExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
- tok::TokenKind OpKind, CXXScopeSpec &SS,
+ bool IsArrow, CXXScopeSpec &SS,
SourceLocation TemplateKWLoc,
UnqualifiedId &Member, Decl *ObjCImpDecl);
@@ -6951,15 +6952,14 @@ class Sema final : public SemaBase {
ExprResult BuildMemberReferenceExpr(
Expr *Base, QualType BaseType, SourceLocation OpLoc, bool IsArrow,
CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
- NamedDecl *FirstQualifierInScope, const DeclarationNameInfo &NameInfo,
+ const DeclarationNameInfo &NameInfo,
const TemplateArgumentListInfo *TemplateArgs, const Scope *S,
ActOnMemberAccessExtraArgs *ExtraArgs = nullptr);
ExprResult
BuildMemberReferenceExpr(Expr *Base, QualType BaseType, SourceLocation OpLoc,
bool IsArrow, const CXXScopeSpec &SS,
- SourceLocation TemplateKWLoc,
- NamedDecl *FirstQualifierInScope, LookupResult &R,
+ SourceLocation TemplateKWLoc, LookupResult &R,
const TemplateArgumentListInfo *TemplateArgs,
const Scope *S, bool SuppressQualifierCheck = false,
ActOnMemberAccessExtraArgs *ExtraArgs = nullptr);
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index cab5ee6047956..b47d7e86013dd 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -8432,8 +8432,14 @@ ExpectedStmt ASTNodeImporter::VisitCXXDependentScopeMemberExpr(
auto ToOperatorLoc = importChecked(Err, E->getOperatorLoc());
auto ToQualifierLoc = importChecked(Err, E->getQualifierLoc());
auto ToTemplateKeywordLoc = importChecked(Err, E->getTemplateKeywordLoc());
- auto ToFirstQualifierFoundInScope =
- importChecked(Err, E->getFirstQualifierFoundInScope());
+
+ UnresolvedSet<8> ToUnqualifiedLookups;
+ for (auto D : E->unqualified_lookups())
+ if (auto ToDOrErr = import(D.getDecl()))
+ ToUnqualifiedLookups.addDecl(*ToDOrErr);
+ else
+ return ToDOrErr.takeError();
+
if (Err)
return std::move(Err);
@@ -8467,7 +8473,7 @@ ExpectedStmt ASTNodeImporter::VisitCXXDependentScopeMemberExpr(
return CXXDependentScopeMemberExpr::Create(
Importer.getToContext(), ToBase, ToType, E->isArrow(), ToOperatorLoc,
- ToQualifierLoc, ToTemplateKeywordLoc, ToFirstQualifierFoundInScope,
+ ToQualifierLoc, ToTemplateKeywordLoc, ToUnqualifiedLookups.pairs(),
ToMemberNameInfo, ResInfo);
}
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 2abc0acbfde3b..826f66fe1624c 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1442,19 +1442,27 @@ SourceLocation CXXUnresolvedConstructExpr::getBeginLoc() const {
CXXDependentScopeMemberExpr::CXXDependentScopeMemberExpr(
const ASTContext &Ctx, Expr *Base, QualType BaseType, bool IsArrow,
SourceLocation OperatorLoc, NestedNameSpecifierLoc QualifierLoc,
- SourceLocation TemplateKWLoc, NamedDecl *FirstQualifierFoundInScope,
+ SourceLocation TemplateKWLoc, ArrayRef<DeclAccessPair> UnqualifiedLookups,
DeclarationNameInfo MemberNameInfo,
const TemplateArgumentListInfo *TemplateArgs)
: Expr(CXXDependentScopeMemberExprClass, Ctx.DependentTy, VK_LValue,
OK_Ordinary),
- Base(Base), BaseType(BaseType), QualifierLoc(QualifierLoc),
+ Base(Base), BaseType(BaseType), OperatorLoc(OperatorLoc),
MemberNameInfo(MemberNameInfo) {
CXXDependentScopeMemberExprBits.IsArrow = IsArrow;
+ CXXDependentScopeMemberExprBits.HasQualifier = QualifierLoc.hasQualifier();
+ CXXDependentScopeMemberExprBits.NumUnqualifiedLookups =
+ UnqualifiedLookups.size();
CXXDependentScopeMemberExprBits.HasTemplateKWAndArgsInfo =
(TemplateArgs != nullptr) || TemplateKWLoc.isValid();
- CXXDependentScopeMemberExprBits.HasFirstQualifierFoundInScope =
- FirstQualifierFoundInScope != nullptr;
- CXXDependentScopeMemberExprBits.OperatorLoc = OperatorLoc;
+
+ if (hasQualifier())
+ new (getTrailingObjects<NestedNameSpecifierLoc>())
+ NestedNameSpecifierLoc(QualifierLoc);
+
+ std::uninitialized_copy_n(UnqualifiedLookups.data(),
+ UnqualifiedLookups.size(),
+ getTrailingObjects<DeclAccessPair>());
if (TemplateArgs) {
auto Deps = TemplateArgumentDependence::None;
@@ -1466,54 +1474,59 @@ CXXDependentScopeMemberExpr::CXXDependentScopeMemberExpr(
TemplateKWLoc);
}
- if (hasFirstQualifierFoundInScope())
- *getTrailingObjects<NamedDecl *>() = FirstQualifierFoundInScope;
setDependence(computeDependence(this));
}
CXXDependentScopeMemberExpr::CXXDependentScopeMemberExpr(
- EmptyShell Empty, bool HasTemplateKWAndArgsInfo,
- bool HasFirstQualifierFoundInScope)
+ EmptyShell Empty, bool HasQualifier, unsigned NumUnqualifiedLookups,
+ bool HasTemplateKWAndArgsInfo)
: Expr(CXXDependentScopeMemberExprClass, Empty) {
+ CXXDependentScopeMemberExprBits.HasQualifier = HasQualifier;
+ CXXDependentScopeMemberExprBits.NumUnqualifiedLookups = NumUnqualifiedLookups;
CXXDependentScopeMemberExprBits.HasTemplateKWAndArgsInfo =
HasTemplateKWAndArgsInfo;
- CXXDependentScopeMemberExprBits.HasFirstQualifierFoundInScope =
- HasFirstQualifierFoundInScope;
}
CXXDependentScopeMemberExpr *CXXDependentScopeMemberExpr::Create(
const ASTContext &Ctx, Expr *Base, QualType BaseType, bool IsArrow,
SourceLocation OperatorLoc, NestedNameSpecifierLoc QualifierLoc,
- SourceLocation TemplateKWLoc, NamedDecl *FirstQualifierFoundInScope,
+ SourceLocation TemplateKWLoc, ArrayRef<DeclAccessPair> UnqualifiedLookups,
DeclarationNameInfo MemberNameInfo,
const TemplateArgumentListInfo *TemplateArgs) {
+ bool HasQualifier = QualifierLoc.hasQualifier();
+ unsigned NumUnqualifiedLookups = UnqualifiedLookups.size();
+ assert(!NumUnqualifiedLookups || HasQualifier);
bool HasTemplateKWAndArgsInfo =
(TemplateArgs != nullptr) || TemplateKWLoc.isValid();
unsigned NumTemplateArgs = TemplateArgs ? TemplateArgs->size() : 0;
- bool HasFirstQualifierFoundInScope = FirstQualifierFoundInScope != nullptr;
-
- unsigned Size = totalSizeToAlloc<ASTTemplateKWAndArgsInfo,
- TemplateArgumentLoc, NamedDecl *>(
- HasTemplateKWAndArgsInfo, NumTemplateArgs, HasFirstQualifierFoundInScope);
+ unsigned Size =
+ totalSizeToAlloc<NestedNameSpecifierLoc, DeclAccessPair,
+ ASTTemplateKWAndArgsInfo, TemplateArgumentLoc>(
+ HasQualifier, NumUnqualifiedLookups, HasTemplateKWAndArgsInfo,
+ NumTemplateArgs);
void *Mem = Ctx.Allocate(Size, alignof(CXXDependentScopeMemberExpr));
return new (Mem) CXXDependentScopeMemberExpr(
Ctx, Base, BaseType, IsArrow, OperatorLoc, QualifierLoc, TemplateKWLoc,
- FirstQualifierFoundInScope, MemberNameInfo, TemplateArgs);
+ UnqualifiedLookups, MemberNameInfo, TemplateArgs);
}
CXXDependentScopeMemberExpr *CXXDependentScopeMemberExpr::CreateEmpty(
- const ASTContext &Ctx, bool HasTemplateKWAndArgsInfo,
- unsigned NumTemplateArgs, bool HasFirstQualifierFoundInScope) {
- assert(NumTemplateArgs == 0 || HasTemplateKWAndArgsInfo);
+ const ASTContext &Ctx, bool HasQualifier, unsigned NumUnqualifiedLookups,
+ bool HasTemplateKWAndArgsInfo, unsigned NumTemplateArgs) {
+ assert(!NumTemplateArgs || HasTemplateKWAndArgsInfo);
+ assert(!NumUnqualifiedLookups || HasQualifier);
- unsigned Size = totalSizeTo...
[truncated]
|
|
I still need to add more comments and a release note (and do some cleanups), but this is otherwise functionally complete |
|
Ping @erichkeane @shafik @Endilll (not sure if drafts send emails for review requests) |
sdkrystian
force-pushed
the
cwg-1835-rev-3
branch
from
ad0ad1c
to
ebe263f
Compare
Per my reading, type-only lookup is performed only for elaborated type specifiers (http://eel.is/c++draft/basic.lookup#elab-1.sentence-1) and base-specifiers (http://eel.is/c++draft/class.derived#general-2.sentence-4). What [basic.lookup.qual.general] p1 describes is much like type-only lookup, but namespaces are considered. Another important difference is that http://eel.is/c++draft/basic.lookup#general-4.sentence-2 does apply to lookup of identifiers followed by Not saying that this materially changes anything, but we should be more cautious with terminology. |
If I understand correctly, you point to a conflict between http://eel.is/c++draft/basic.lookup#qual.general-3.sentence-3 and http://eel.is/c++draft/temp.res#temp.dep.type-6. Have you considered filing a Core issue? |
@Endilll Sorry, I used "type-only" there but I intended to write "only considers namespaces, types, and templates whose specializations are types". I'll edit my comment to fix that. The intended phrasing is what I implemented. |
|
This does seem to cause an error in libstdc++: Since |
sdkrystian
force-pushed
the
cwg-1835-rev-3
branch
2 times, most recently
from
42d9da7
to
13ff865
Compare
jcsxky
reviewed
Jun 4, 2024
sdkrystian
force-pushed
the
cwg-1835-rev-3
branch
from
4b1fd30
to
211afbf
Compare
sdkrystian
force-pushed
the
cwg-1835-rev-3
branch
from
8def09c
to
347e486
Compare
sdkrystian
force-pushed
the
cwg-1835-rev-3
branch
from
1d9c473
to
24f1447
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/862 Here is the relevant piece of the build log for the reference: |
chapuni
added a commit
that referenced
this pull request
Jul 10, 2024
ppc64le-lld-multistage-test has been failing. This reverts commit 7bfb98c.
|
Reverted, thank you. |
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Jul 11, 2024
sdkrystian
added a commit
that referenced
this pull request
Jul 11, 2024
Reapplies #92957, fixing an instance where the `template` keyword was missing prior to a dependent name in `llvm/ADT/ArrayRef.h`. An _alias-declaration_ is used to work around a bug affecting GCC releases before 11.1 (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94799) which rejects the use of the `template` keyword prior to the _nested-name-specifier_ in the class member access.
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
CWG1835 was one of the many core issues resolved by P1787R6: "Declarations and where to find them" (http://wg21.link/p1787r6). Its resolution changes how member-qualified names (as defined by [basic.lookup.qual.general] p2) are looked up. This patch implementation that resolution. Previously, an _identifier_ following `.` or `->` would be first looked up in the type of the object expression (i.e. qualified lookup), and then in the context of the _postfix-expression_ (i.e. unqualified lookup) if nothing was found; the result of the second lookup was required to name a class template. Notably, this second lookup would occur even when the object expression was dependent, and its result would be used to determine whether a `<` token is the start of a _template-argument_list_. The new wording in [basic.lookup.qual.general] p2 states: > A member-qualified name is the (unique) component name, if any, of > - an _unqualified-id_ or > - a _nested-name-specifier_ of the form _`type-name ::`_ or _`namespace-name ::`_ > > in the id-expression of a class member access expression. A ***qualified name*** is > - a member-qualified name or > - the terminal name of > - a _qualified-id_, > - a _using-declarator_, > - a _typename-specifier_, > - a _qualified-namespace-specifier_, or > - a _nested-name-specifier_, _elaborated-type-specifier_, or _class-or-decltype_ that has a _nested-name-specifier_. > > The _lookup context_ of a member-qualified name is the type of its associated object expression (considered dependent if the object expression is type-dependent). The lookup context of any other qualified name is the type, template, or namespace nominated by the preceding _nested-name-specifier_. And [basic.lookup.qual.general] p3 now states: > _Qualified name lookup_ in a class, namespace, or enumeration performs a search of the scope associated with it except as specified below. Unless otherwise specified, a qualified name undergoes qualified name lookup in its lookup context from the point where it appears unless the lookup context either is dependent and is not the current instantiation or is not a class or class template. If nothing is found by qualified lookup for a member-qualified name that is the terminal name of a _nested-name-specifier_ and is not dependent, it undergoes unqualified lookup. In non-standardese terms, these two paragraphs essentially state the following: - A name that immediately follows `.` or `->` in a class member access expression is a member-qualified name - A member-qualified name will be first looked up in the type of the object expression `T` unless `T` is a dependent type that is _not_ the current instantiation, e.g. ``` template<typename T> struct A { void f(T* t) { this->x; // type of the object expression is 'A<T>'. although 'A<T>' is dependent, it is the // current instantiation so we look up 'x' in the template definition context. t->y; // type of the object expression is 'T' ('->' is transformed to '.' per [expr.ref]). // 'T' is dependent and is *not* the current instantiation, so we lookup 'y' in the // template instantiation context. } }; ``` - If the first lookup finds nothing and: - the member-qualified name is the first component of a _nested-name-specifier_ (which could be an _identifier_ or a _simple-template-id_), and either: - the type of the object expression is the current instantiation and it has no dependent base classes, or - the type of the object expression is not dependent then we lookup the name again, this time via unqualified lookup. Although the second (unqualified) lookup is stated not to occur when the member-qualified name is dependent, a dependent name will _not_ be dependent once the template is instantiated, so the second lookup must "occur" during instantiation if qualified lookup does not find anything. This means that we must perform the second (unqualified) lookup during parsing even when the type of the object expression is dependent, but those results are _not_ used to determine whether a `<` token is the start of a _template-argument_list_; they are stored so we can replicate the second lookup during instantiation. In even simpler terms (paraphrasing the meeting minutes from the review of P1787; see https://wiki.edg.com/bin/view/Wg21summer2020/P1787%28Lookup%29Review2020-06-15Through2020-06-18): - Unqualified lookup always happens for the first name in a _nested-name-specifier_ that follows `.` or `->` - The result of that lookup is only used to determine whether `<` is the start of a _template-argument-list_ if the first (qualified) lookup found nothing and the lookup context: - is not dependent, or - is the current instantiation and has no dependent base classes. An example: ``` struct A { void f(); }; template<typename T> using B = A; template<typename T> struct C : A { template<typename U> void g(); void h(T* t) { this->g<int>(); // ok, '<' is the start of a template-argument-list ('g' was found via qualified lookup in the current instantiation) this->B<void>::f(); // ok, '<' is the start of a template-argument-list (current instantiation has no dependent bases, 'B' was found via unqualified lookup) t->g<int>(); // error: '<' means less than (unqualified lookup does not occur for a member-qualified name that isn't the first component of a nested-name-specifier) t->B<void>::f(); // error: '<' means less than (unqualified lookup does not occur if the name is dependent) t->template B<void>::f(); // ok: '<' is the start of a template-argument-list ('template' keyword used) } }; ``` Some additional notes: - Per [basic.lookup.qual.general] p1, lookup for a member-qualified name only considers namespaces, types, and templates whose specializations are types if it's an _identifier_ followed by `::`; lookup for the component name of a _simple-template-id_ followed by `::` is _not_ subject to this rule. - The wording which specifies when the second unqualified lookup occurs appears to be paradoxical. We are supposed to do it only for the first component name of a _nested-name-specifier_ that follows `.` or `->` when qualified lookup finds nothing. However, when that name is followed by `<` (potentially starting a _simple-template-id_) we don't _know_ whether it will be the start of a _nested-name-specifier_ until we do the lookup -- but we aren't supposed to do the lookup until we know it's part of a _nested-name-specifier_! ***However***, since we only do the second lookup when the first lookup finds nothing (and the name isn't dependent), ***and*** since neither lookup is type-only, the only valid option is for the name to be the _template-name_ in a _simple-template-id_ that is followed by `::` (it can't be an _unqualified-id_ naming a member because we already determined that the lookup context doesn't have a member with that name). Thus, we can lock into the _nested-name-specifier_ interpretation and do the second lookup without having to know whether the _simple-template-id_ will be followed by `::` yet.
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
ppc64le-lld-multistage-test has been failing. This reverts commit 7bfb98c.
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
…#98547) Reapplies llvm#92957, fixing an instance where the `template` keyword was missing prior to a dependent name in `llvm/ADT/ArrayRef.h`. An _alias-declaration_ is used to work around a bug affecting GCC releases before 11.1 (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94799) which rejects the use of the `template` keyword prior to the _nested-name-specifier_ in the class member access.
hokein
added a commit
that referenced
this pull request
Jul 15, 2024
…98547)" This reverts commit ce4aada and a follow-up patch 8ef26f1. This new warning can not be fully suppressed by the `-Wno-missing-dependent-template-keyword` flag, this gives developer no time to do the cleanup in a large codebase, see #98547 (comment)
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Jul 18, 2024
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Jul 26, 2024
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Jul 29, 2024
sdkrystian
added a commit
to sdkrystian/llvm-project
that referenced
this pull request
Jul 30, 2024
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.
CWG1835 was one of the many core issues resolved by P1787R6: Declarations and where to find them. Its resolution changes how member-qualified names are looked up. This patch is a draft implementation of that resolution.
Previously, an identifier following
.or->would be first looked up in the type of the object expression (i.e. qualified lookup), and then in the context of the postfix-expression (i.e. unqualified lookup) if nothing was found; the result of the second lookup was required to name a class template. Notably, this second lookup would occur even when the object expression was dependent, and its result would be used to determine whether a<token is the start of a template-argument_list.The new wording in [basic.lookup.qual.general] p2 states:
And [basic.lookup.qual.general] p3 now states:
In non-standardese terms, these two paragraphs essentially state the following:
.or->in a class member access expression is a member-qualified nameTunlessTis a dependent type that is not the current instantiation, e.g.If the first lookup finds nothing and:
then we lookup the name again, this time via unqualified lookup.
Although the second (unqualified) lookup is stated not to occur when the member-qualified name is dependent, a dependent name will not be dependent once the template is instantiated, so the second lookup must "occur" during instantiation if qualified lookup does not find anything. This means that we must perform the second (unqualified) lookup during parsing even when the type of the object expression is dependent, but those results are not used to determine whether a
<token is the start of a template-argument_list; they are stored so we can replicate the second lookup during instantiation.In even simpler terms (paraphrasing the meeting minutes from the review of P1787):
.or-><is the start of a template-argument-list if the first (qualified) lookup found nothing and the lookup context:An example:
Some additional notes:
::; lookup for the component name of a simple-template-id followed by::is not subject to this rule..or->when qualified lookup finds nothing. However, when that name is followed by<(potentially starting a simple-template-id) we don't know whether it will be the start of a nested-name-specifier until we do the lookup -- but we aren't supposed to do the lookup until we know it's part of a nested-name-specifier! However, since we only do the second lookup when the first lookup finds nothing (and the name isn't dependent), and since neither lookup is type-only, the only valid option is for the name to be the template-name in a simple-template-id that is followed by::(it can't be an unqualified-id naming a member because we already determined that the lookup context doesn't have a member with that name). Thus, we can lock into the nested-name-specifier interpretation and do the second lookup without having to know whether the simple-template-id will be followed by::yet.