Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Implement CWG2398 provisional TTP matching to class templates #94981

Merged
1 change: 1 addition & 0 deletions clang-tools-extra/clangd/DumpAST.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ class DumpVisitor : public RecursiveASTVisitor<DumpVisitor> {
TEMPLATE_KIND(SubstTemplateTemplateParm);
TEMPLATE_KIND(SubstTemplateTemplateParmPack);
TEMPLATE_KIND(UsingTemplate);
TEMPLATE_KIND(DeducedTemplate);
#undef TEMPLATE_KIND
}
llvm_unreachable("Unhandled NameKind enum");
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/clangd/SemanticHighlighting.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,7 @@ class CollectExtraHighlightings
case TemplateName::SubstTemplateTemplateParm:
case TemplateName::SubstTemplateTemplateParmPack:
case TemplateName::UsingTemplate:
case TemplateName::DeducedTemplate:
// Names that could be resolved to a TemplateDecl are handled elsewhere.
break;
}
Expand Down
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ Resolutions to C++ Defect Reports
- Allow ``void{}`` as a prvalue of type ``void``.
(`CWG2351: void{} <https://cplusplus.github.io/CWG/issues/2351.html>`_).

- Clang now has improved resolution to CWG2398, allowing class templates to have
default arguments deduced when partial ordering.

C Language Changes
------------------

Expand Down
11 changes: 9 additions & 2 deletions clang/include/clang/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
mutable llvm::ContextualFoldingSet<SubstTemplateTemplateParmPackStorage,
ASTContext&>
SubstTemplateTemplateParmPacks;
mutable llvm::ContextualFoldingSet<DeducedTemplateStorage, ASTContext &>
DeducedTemplates;

mutable llvm::ContextualFoldingSet<ArrayParameterType, ASTContext &>
ArrayParameterTypes;
Expand Down Expand Up @@ -2291,6 +2293,9 @@ class ASTContext : public RefCountedBase<ASTContext> {
unsigned Index,
bool Final) const;

TemplateName getDeducedTemplateName(TemplateName Underlying,
DefaultArguments DefaultArgs) const;

enum GetBuiltinTypeError {
/// No error
GE_None,
Expand Down Expand Up @@ -2774,11 +2779,13 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// template name uses the shortest form of the dependent
/// nested-name-specifier, which itself contains all canonical
/// types, values, and templates.
TemplateName getCanonicalTemplateName(const TemplateName &Name) const;
TemplateName getCanonicalTemplateName(TemplateName Name,
bool IgnoreDeduced = false) const;

/// Determine whether the given template names refer to the same
/// template.
bool hasSameTemplateName(const TemplateName &X, const TemplateName &Y) const;
bool hasSameTemplateName(const TemplateName &X, const TemplateName &Y,
bool IgnoreDeduced = false) const;

/// Determine whether the two declarations refer to the same entity.
bool isSameEntity(const NamedDecl *X, const NamedDecl *Y) const;
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/AST/ASTImporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,11 @@ class TypeSourceInfo;
/// the declarations it contains.
[[nodiscard]] llvm::Error ImportDefinition(Decl *From);

llvm::Error
ImportTemplateArguments(ArrayRef<TemplateArgument> FromArgs,
SmallVectorImpl<TemplateArgument> &ToArgs);
Expected<TemplateArgument> Import(const TemplateArgument &From);

/// Cope with a name conflict when importing a declaration into the
/// given context.
///
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/AST/DependenceFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,11 @@ toTemplateNameDependence(NestedNameSpecifierDependence D) {
return Dependence(D).templateName();
}

inline TemplateNameDependence
toTemplateNameDependence(TemplateArgumentDependence D) {
return Dependence(D).templateName();
}

LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();

} // namespace clang
Expand Down
17 changes: 17 additions & 0 deletions clang/include/clang/AST/PropertiesBase.td
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,23 @@ let Class = PropertyTypeCase<TemplateName, "SubstTemplateTemplateParmPack"> in {
return ctx.getSubstTemplateTemplateParmPack(argumentPack, associatedDecl, index, final);
}]>;
}
let Class = PropertyTypeCase<TemplateName, "DeducedTemplate"> in {
def : ReadHelper<[{
auto DTS = node.getAsDeducedTemplateName();
}]>;
def : Property<"underlying", TemplateName> {
let Read = [{ DTS->getUnderlying() }];
}
def : Property<"startPos", UInt32> {
let Read = [{ DTS->getDefaultArguments().StartPos }];
}
def : Property<"defaultArgs", Array<TemplateArgument>> {
let Read = [{ DTS->getDefaultArguments().Args }];
}
def : Creator<[{
return ctx.getDeducedTemplateName(underlying, {startPos, defaultArgs});
}]>;
}

// Type cases for TemplateArgument.
def : PropertyTypeKind<TemplateArgument, TemplateArgumentKind,
Expand Down
63 changes: 60 additions & 3 deletions clang/include/clang/AST/TemplateName.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class NestedNameSpecifier;
enum OverloadedOperatorKind : int;
class OverloadedTemplateStorage;
class AssumedTemplateStorage;
class DeducedTemplateStorage;
struct PrintingPolicy;
class QualifiedTemplateName;
class SubstTemplateTemplateParmPackStorage;
Expand All @@ -50,16 +51,17 @@ class UncommonTemplateNameStorage {
enum Kind {
Overloaded,
Assumed, // defined in DeclarationName.h
Deduced,
SubstTemplateTemplateParm,
SubstTemplateTemplateParmPack
};

struct BitsTag {
LLVM_PREFERRED_TYPE(Kind)
unsigned Kind : 2;
unsigned Kind : 3;

// The template parameter index.
unsigned Index : 15;
unsigned Index : 14;

/// The pack index, or the number of stored templates
/// or template arguments, depending on which subclass we have.
Expand Down Expand Up @@ -90,6 +92,12 @@ class UncommonTemplateNameStorage {
: nullptr;
}

DeducedTemplateStorage *getAsDeducedTemplateName() {
return Bits.Kind == Deduced
? reinterpret_cast<DeducedTemplateStorage *>(this)
: nullptr;
}

SubstTemplateTemplateParmStorage *getAsSubstTemplateTemplateParm() {
return Bits.Kind == SubstTemplateTemplateParm
? reinterpret_cast<SubstTemplateTemplateParmStorage *>(this)
Expand Down Expand Up @@ -172,6 +180,15 @@ class SubstTemplateTemplateParmPackStorage : public UncommonTemplateNameStorage,
unsigned Index, bool Final);
};

struct DefaultArguments {
// The position in the template parameter list
// the first argument corresponds to.
unsigned StartPos;
mizvekov marked this conversation as resolved.
Show resolved Hide resolved
ArrayRef<TemplateArgument> Args;

operator bool() const { return !Args.empty(); }
};

/// Represents a C++ template name within the type system.
///
/// A C++ template name refers to a template within the C++ type
Expand Down Expand Up @@ -246,6 +263,10 @@ class TemplateName {
/// A template name that refers to a template declaration found through a
/// specific using shadow declaration.
UsingTemplate,

/// A template name that refers to another TemplateName with deduced default
/// arguments.
DeducedTemplate,
};

TemplateName() = default;
Expand All @@ -257,6 +278,7 @@ class TemplateName {
explicit TemplateName(QualifiedTemplateName *Qual);
explicit TemplateName(DependentTemplateName *Dep);
explicit TemplateName(UsingShadowDecl *Using);
explicit TemplateName(DeducedTemplateStorage *Deduced);

/// Determine whether this template name is NULL.
bool isNull() const;
Expand All @@ -271,7 +293,13 @@ class TemplateName {
/// to, if any. If the template name does not refer to a specific
/// declaration because it is a dependent name, or if it refers to a
/// set of function templates, returns NULL.
TemplateDecl *getAsTemplateDecl() const;
TemplateDecl *getAsTemplateDecl(bool IgnoreDeduced = false) const;

/// Retrieves the underlying template declaration that
/// this template name refers to, along with the
/// deduced default arguments, if any.
std::pair<TemplateDecl *, DefaultArguments>
getTemplateDeclAndDefaultArgs() const;

/// Retrieve the underlying, overloaded function template
/// declarations that this template name refers to, if known.
Expand Down Expand Up @@ -313,6 +341,11 @@ class TemplateName {
/// template declaration is introduced, if any.
UsingShadowDecl *getAsUsingShadowDecl() const;

/// Retrieve the deduced template info, if any.
DeducedTemplateStorage *getAsDeducedTemplateName() const;

std::optional<TemplateName> desugar(bool IgnoreDeduced) const;

TemplateName getUnderlying() const;

TemplateNameDependence getDependence() const;
Expand Down Expand Up @@ -412,6 +445,30 @@ class SubstTemplateTemplateParmStorage
std::optional<unsigned> PackIndex);
};

class DeducedTemplateStorage : public UncommonTemplateNameStorage,
mizvekov marked this conversation as resolved.
Show resolved Hide resolved
public llvm::FoldingSetNode {
friend class ASTContext;

TemplateName Underlying;

DeducedTemplateStorage(TemplateName Underlying,
const DefaultArguments &DefArgs);

public:
TemplateName getUnderlying() const { return Underlying; }

DefaultArguments getDefaultArguments() const {
return {/*StartPos=*/Bits.Index,
/*Args=*/{reinterpret_cast<const TemplateArgument *>(this + 1),
Bits.Data}};
}

void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context) const;

static void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
TemplateName Underlying, const DefaultArguments &DefArgs);
};

inline TemplateName TemplateName::getUnderlying() const {
if (SubstTemplateTemplateParmStorage *subst
= getAsSubstTemplateTemplateParm())
Expand Down
10 changes: 7 additions & 3 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -11728,6 +11728,9 @@ class Sema final : public SemaBase {
/// receive true if the cause for the error is the associated constraints of
/// the template not being satisfied by the template arguments.
///
/// \param DefaultArgs any default arguments from template specialization
/// deduction.
///
/// \param PartialOrderingTTP If true, assume these template arguments are
/// the injected template arguments for a template template parameter.
/// This will relax the requirement that all its possible uses are valid:
Expand All @@ -11737,7 +11740,8 @@ class Sema final : public SemaBase {
/// \returns true if an error occurred, false otherwise.
bool CheckTemplateArgumentList(
TemplateDecl *Template, SourceLocation TemplateLoc,
TemplateArgumentListInfo &TemplateArgs, bool PartialTemplateArgs,
TemplateArgumentListInfo &TemplateArgs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead we want to add an additional method.
In a lot of places through the rest of the patch, DefaultArgs is defaulted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing a worthwhile tradeoff.

Just the function signature is hugely complicated, and would need to be duplicated.
The implementation would be mostly the same, with a small block which would be omitted in one of the implementations.

What did you have in mind?

Copy link
Contributor

@cor3ntin cor3ntin Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool CheckTemplateArgumentList(
      TemplateDecl *Template, SourceLocation TemplateLoc,
      TemplateArgumentListInfo &TemplateArgs,
      bool PartialTemplateArgs,
      SmallVectorImpl<TemplateArgument> &SugaredConverted,
      SmallVectorImpl<TemplateArgument> &CanonicalConverted,
      bool UpdateArgsWithConversions = true,
      bool *ConstraintsNotSatisfied = nullptr, bool PartialOrderingTTP = false, const DefaultArguments * DefaultArgs = nullptr);


bool CheckTemplateArgumentList(
      TemplateDecl *Template, SourceLocation TemplateLoc,
      TemplateArgumentListInfo &TemplateArgs,
      const DefaultArguments & DefaultArgs, 
      SmallVectorImpl<TemplateArgument> &SugaredConverted,
      SmallVectorImpl<TemplateArgument> &CanonicalConverted) {

return CheckTemplateArgumentList(Template, TemplateLoc,
                                             /*PartialTemplateArgs=*/false,
                                              SugaredConverted, CanonicalConverted,
                                              /*UpdateArgsWithConversions=*/true, 
                                               nullptr, false, &DefaultArgs); 
                                                                 
}

Maybe something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. One issue I have often had with these functions with large amount of both defaulted and non-defaulted parameters, is that you would want to extend it by changing the signature, then arguments would match parameters incorrectly, but this would not cause a hard error on all of the call sites.

I could have easily added DefaultArgs as defaulted empty here, but chose not to due to this reason.

Besides that, overloading functions with such huge numbers of parameters creates some confusion as well.
I'd slightly prefer if we avoided that, but don't have strong enough feelings to go on a crusade against it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we think about ways to simplify these interface in a subsequent patch?
@AaronBallman @erichkeane for additional opinions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's enough repeated stuff though these interfaces here, I don't think I'd mind a followup that created an object to contain all the related stuff. For example, the Sugared/Canonical vectors shoudl probably be a vectro of TempalteArgument pairs (or more likely their own structure) or something? But I'm open to other ideas as well (agian in a followup).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the separate Sugared / Converted lists I already have an incomplete patch to address that, though I am currently working on something else.

const DefaultArguments &DefaultArgs, bool PartialTemplateArgs,
SmallVectorImpl<TemplateArgument> &SugaredConverted,
SmallVectorImpl<TemplateArgument> &CanonicalConverted,
bool UpdateArgsWithConversions = true,
Expand Down Expand Up @@ -12474,8 +12478,8 @@ class Sema final : public SemaBase {
sema::TemplateDeductionInfo &Info);

bool isTemplateTemplateParameterAtLeastAsSpecializedAs(
TemplateParameterList *PParam, TemplateDecl *AArg, SourceLocation Loc,
bool IsDeduced);
TemplateParameterList *PParam, TemplateDecl *AArg,
const DefaultArguments &DefaultArgs, SourceLocation Loc, bool IsDeduced);

/// Mark which template parameters are used in a given expression.
///
Expand Down
Loading
Loading