[c++20] P1907R1: Support for generalized non-type template arguments of scalar type. #78041
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-lldb Author: Andrey Ali Khan Bolshakov (bolshakov-a) ChangesPreviously committed as 9e08e51, and reverted because a dependency commit was reverted, then committed again as 4b57400 and reverted again because "dependency commit" 5a391d3 was reverted. But it doesn't seem that 5a391d3 was a real dependency for this. This commit incorporates 4b57400 and 18e093f by Richard Smith (@zygoloid), with some minor fixes, most notably:
Patch is 101.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/78041.diff 47 Files Affected:
diff --git a/clang-tools-extra/clangd/DumpAST.cpp b/clang-tools-extra/clangd/DumpAST.cpp
index b0cec65c39fa31..9a525efb938e8d 100644
--- a/clang-tools-extra/clangd/DumpAST.cpp
+++ b/clang-tools-extra/clangd/DumpAST.cpp
@@ -143,6 +143,7 @@ class DumpVisitor : public RecursiveASTVisitor<DumpVisitor> {
TEMPLATE_ARGUMENT_KIND(Declaration);
TEMPLATE_ARGUMENT_KIND(Template);
TEMPLATE_ARGUMENT_KIND(TemplateExpansion);
+ TEMPLATE_ARGUMENT_KIND(StructuralValue);
#undef TEMPLATE_ARGUMENT_KIND
}
llvm_unreachable("Unhandled ArgKind enum");
diff --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index 839cf6332fe8b0..a4a261030d399b 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -1032,6 +1032,7 @@ class ExplicitReferenceCollector
case TemplateArgument::Pack:
case TemplateArgument::Type:
case TemplateArgument::Expression:
+ case TemplateArgument::StructuralValue:
break; // Handled by VisitType and VisitExpression.
};
return RecursiveASTVisitor::TraverseTemplateArgumentLoc(A);
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3cbce1be159437..57c444fd95be1b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -180,6 +180,8 @@ C++ Language Changes
C++20 Feature Support
^^^^^^^^^^^^^^^^^^^^^
+- Implemented `P1907R1 <https://wg21.link/P1907R1>` which extends allowed non-type template argument
+ kinds with e.g. floating point values and pointers and references to subobjects.
C++23 Feature Support
^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ODRHash.h b/clang/include/clang/AST/ODRHash.h
index cedf644520fc32..a1caa6d39a87c3 100644
--- a/clang/include/clang/AST/ODRHash.h
+++ b/clang/include/clang/AST/ODRHash.h
@@ -25,6 +25,7 @@
namespace clang {
+class APValue;
class Decl;
class IdentifierInfo;
class NestedNameSpecifier;
@@ -101,6 +102,8 @@ class ODRHash {
// Save booleans until the end to lower the size of data to process.
void AddBoolean(bool value);
+ void AddStructuralValue(const APValue &);
+
static bool isSubDeclToBeProcessed(const Decl *D, const DeclContext *Parent);
private:
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index d86c4eba6a2251..0270c086d06b6a 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -808,6 +808,20 @@ let Class = PropertyTypeCase<TemplateArgument, "Integral"> in {
return TemplateArgument(ctx, value, type, isDefaulted);
}]>;
}
+let Class = PropertyTypeCase<TemplateArgument, "StructuralValue"> in {
+ def : Property<"value", APValue> {
+ let Read = [{ node.getAsStructuralValue() }];
+ }
+ def : Property<"type", QualType> {
+ let Read = [{ node.getStructuralValueType() }];
+ }
+ def : Property<"isDefaulted", Bool> {
+ let Read = [{ node.getIsDefaulted() }];
+ }
+ def : Creator<[{
+ return TemplateArgument(ctx, type, value, isDefaulted);
+ }]>;
+}
let Class = PropertyTypeCase<TemplateArgument, "Template"> in {
def : Property<"name", TemplateName> {
let Read = [{ node.getAsTemplateOrTemplatePattern() }];
diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index 8f2714e142bbe3..2aee6a947141b6 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -850,6 +850,7 @@ bool RecursiveASTVisitor<Derived>::TraverseTemplateArgument(
case TemplateArgument::Declaration:
case TemplateArgument::Integral:
case TemplateArgument::NullPtr:
+ case TemplateArgument::StructuralValue:
return true;
case TemplateArgument::Type:
@@ -882,6 +883,7 @@ bool RecursiveASTVisitor<Derived>::TraverseTemplateArgumentLoc(
case TemplateArgument::Declaration:
case TemplateArgument::Integral:
case TemplateArgument::NullPtr:
+ case TemplateArgument::StructuralValue:
return true;
case TemplateArgument::Type: {
diff --git a/clang/include/clang/AST/TemplateArgumentVisitor.h b/clang/include/clang/AST/TemplateArgumentVisitor.h
index 190aa97adf4551..cf0d3220158063 100644
--- a/clang/include/clang/AST/TemplateArgumentVisitor.h
+++ b/clang/include/clang/AST/TemplateArgumentVisitor.h
@@ -37,6 +37,7 @@ class Base {
DISPATCH(Declaration);
DISPATCH(NullPtr);
DISPATCH(Integral);
+ DISPATCH(StructuralValue);
DISPATCH(Template);
DISPATCH(TemplateExpansion);
DISPATCH(Expression);
@@ -59,6 +60,7 @@ class Base {
VISIT_METHOD(Declaration);
VISIT_METHOD(NullPtr);
VISIT_METHOD(Integral);
+ VISIT_METHOD(StructuralValue);
VISIT_METHOD(Template);
VISIT_METHOD(TemplateExpansion);
VISIT_METHOD(Expression);
diff --git a/clang/include/clang/AST/TemplateBase.h b/clang/include/clang/AST/TemplateBase.h
index b7cd71f17c9442..fea2c8ccfee675 100644
--- a/clang/include/clang/AST/TemplateBase.h
+++ b/clang/include/clang/AST/TemplateBase.h
@@ -50,6 +50,7 @@ template <> struct PointerLikeTypeTraits<clang::Expr *> {
namespace clang {
+class APValue;
class ASTContext;
class Expr;
struct PrintingPolicy;
@@ -80,6 +81,13 @@ class TemplateArgument {
/// that was provided for an integral non-type template parameter.
Integral,
+ /// The template argument is a non-type template argument that can't be
+ /// represented by the special-case Declaration, NullPtr, or Integral
+ /// forms. These values are only ever produced by constant evaluation,
+ /// so cannot be dependent.
+ /// TODO: merge Declaration, NullPtr and Integral into this?
+ StructuralValue,
+
/// The template argument is a template name that was provided for a
/// template template parameter.
Template,
@@ -130,6 +138,14 @@ class TemplateArgument {
};
void *Type;
};
+ struct V {
+ LLVM_PREFERRED_TYPE(ArgKind)
+ unsigned Kind : 31;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned IsDefaulted : 1;
+ APValue *Value;
+ void *Type;
+ };
struct A {
LLVM_PREFERRED_TYPE(ArgKind)
unsigned Kind : 31;
@@ -156,11 +172,19 @@ class TemplateArgument {
union {
struct DA DeclArg;
struct I Integer;
+ struct V Value;
struct A Args;
struct TA TemplateArg;
struct TV TypeOrValue;
};
+ void initFromType(QualType T, bool IsNullPtr, bool IsDefaulted);
+ void initFromDeclaration(ValueDecl *D, QualType QT, bool IsDefaulted);
+ void initFromIntegral(const ASTContext &Ctx, const llvm::APSInt &Value,
+ QualType Type, bool IsDefaulted);
+ void initFromStructural(const ASTContext &Ctx, QualType Type,
+ const APValue &V, bool IsDefaulted);
+
public:
/// Construct an empty, invalid template argument.
constexpr TemplateArgument() : TypeOrValue({Null, 0, /* IsDefaulted */ 0}) {}
@@ -168,25 +192,22 @@ class TemplateArgument {
/// Construct a template type argument.
TemplateArgument(QualType T, bool isNullPtr = false,
bool IsDefaulted = false) {
- TypeOrValue.Kind = isNullPtr ? NullPtr : Type;
- TypeOrValue.IsDefaulted = IsDefaulted;
- TypeOrValue.V = reinterpret_cast<uintptr_t>(T.getAsOpaquePtr());
+ initFromType(T, isNullPtr, IsDefaulted);
}
- /// Construct a template argument that refers to a
- /// declaration, which is either an external declaration or a
- /// template declaration.
+ /// Construct a template argument that refers to a (non-dependent)
+ /// declaration.
TemplateArgument(ValueDecl *D, QualType QT, bool IsDefaulted = false) {
- assert(D && "Expected decl");
- DeclArg.Kind = Declaration;
- DeclArg.IsDefaulted = IsDefaulted;
- DeclArg.QT = QT.getAsOpaquePtr();
- DeclArg.D = D;
+ initFromDeclaration(D, QT, IsDefaulted);
}
/// Construct an integral constant template argument. The memory to
/// store the value is allocated with Ctx.
- TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value, QualType Type,
+ TemplateArgument(const ASTContext &Ctx, const llvm::APSInt &Value,
+ QualType Type, bool IsDefaulted = false);
+
+ /// Construct a template argument from an arbitrary constant value.
+ TemplateArgument(const ASTContext &Ctx, QualType Type, const APValue &Value,
bool IsDefaulted = false);
/// Construct an integral constant template argument with the same
@@ -297,7 +318,7 @@ class TemplateArgument {
/// Retrieve the type for a type template argument.
QualType getAsType() const {
assert(getKind() == Type && "Unexpected kind");
- return QualType::getFromOpaquePtr(reinterpret_cast<void*>(TypeOrValue.V));
+ return QualType::getFromOpaquePtr(reinterpret_cast<void *>(TypeOrValue.V));
}
/// Retrieve the declaration for a declaration non-type
@@ -315,7 +336,7 @@ class TemplateArgument {
/// Retrieve the type for null non-type template argument.
QualType getNullPtrType() const {
assert(getKind() == NullPtr && "Unexpected kind");
- return QualType::getFromOpaquePtr(reinterpret_cast<void*>(TypeOrValue.V));
+ return QualType::getFromOpaquePtr(reinterpret_cast<void *>(TypeOrValue.V));
}
/// Retrieve the template name for a template name argument.
@@ -371,6 +392,14 @@ class TemplateArgument {
/// default template parameter.
bool getIsDefaulted() const { return (bool)TypeOrValue.IsDefaulted; }
+ /// Get the value of a StructuralValue.
+ const APValue &getAsStructuralValue() const { return *Value.Value; }
+
+ /// Get the type of a StructuralValue.
+ QualType getStructuralValueType() const {
+ return QualType::getFromOpaquePtr(Value.Type);
+ }
+
/// If this is a non-type template argument, get its type. Otherwise,
/// returns a null QualType.
QualType getNonTypeTemplateArgumentType() const;
@@ -516,6 +545,7 @@ class TemplateArgumentLoc {
assert(Argument.getKind() == TemplateArgument::NullPtr ||
Argument.getKind() == TemplateArgument::Integral ||
Argument.getKind() == TemplateArgument::Declaration ||
+ Argument.getKind() == TemplateArgument::StructuralValue ||
Argument.getKind() == TemplateArgument::Expression);
}
@@ -541,13 +571,9 @@ class TemplateArgumentLoc {
/// - Fetches the full source range of the argument.
SourceRange getSourceRange() const LLVM_READONLY;
- const TemplateArgument &getArgument() const {
- return Argument;
- }
+ const TemplateArgument &getArgument() const { return Argument; }
- TemplateArgumentLocInfo getLocInfo() const {
- return LocInfo;
- }
+ TemplateArgumentLocInfo getLocInfo() const { return LocInfo; }
TypeSourceInfo *getTypeSourceInfo() const {
if (Argument.getKind() != TemplateArgument::Type)
@@ -575,6 +601,11 @@ class TemplateArgumentLoc {
return LocInfo.getAsExpr();
}
+ Expr *getSourceStructuralValueExpression() const {
+ assert(Argument.getKind() == TemplateArgument::StructuralValue);
+ return LocInfo.getAsExpr();
+ }
+
NestedNameSpecifierLoc getTemplateQualifierLoc() const {
if (Argument.getKind() != TemplateArgument::Template &&
Argument.getKind() != TemplateArgument::TemplateExpansion)
@@ -606,8 +637,7 @@ class TemplateArgumentListInfo {
public:
TemplateArgumentListInfo() = default;
- TemplateArgumentListInfo(SourceLocation LAngleLoc,
- SourceLocation RAngleLoc)
+ TemplateArgumentListInfo(SourceLocation LAngleLoc, SourceLocation RAngleLoc)
: LAngleLoc(LAngleLoc), RAngleLoc(RAngleLoc) {}
// This can leak if used in an AST node, use ASTTemplateArgumentListInfo
@@ -626,21 +656,15 @@ class TemplateArgumentListInfo {
return Arguments.data();
}
- llvm::ArrayRef<TemplateArgumentLoc> arguments() const {
- return Arguments;
- }
+ llvm::ArrayRef<TemplateArgumentLoc> arguments() const { return Arguments; }
const TemplateArgumentLoc &operator[](unsigned I) const {
return Arguments[I];
}
- TemplateArgumentLoc &operator[](unsigned I) {
- return Arguments[I];
- }
+ TemplateArgumentLoc &operator[](unsigned I) { return Arguments[I]; }
- void addArgument(const TemplateArgumentLoc &Loc) {
- Arguments.push_back(Loc);
- }
+ void addArgument(const TemplateArgumentLoc &Loc) { Arguments.push_back(Loc); }
};
/// Represents an explicit template argument list in C++, e.g.,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1a79892e40030a..8c74d12e674624 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5138,8 +5138,6 @@ def err_non_type_template_arg_subobject : Error<
"non-type template argument refers to subobject '%0'">;
def err_non_type_template_arg_addr_label_diff : Error<
"template argument / label address difference / what did you expect?">;
-def err_non_type_template_arg_unsupported : Error<
- "sorry, non-type template argument of type %0 is not yet supported">;
def err_template_arg_not_convertible : Error<
"non-type template argument of type %0 cannot be converted to a value "
"of type %1">;
@@ -5191,9 +5189,6 @@ def err_template_arg_not_object_or_func : Error<
"non-type template argument does not refer to an object or function">;
def err_template_arg_not_pointer_to_member_form : Error<
"non-type template argument is not a pointer to member constant">;
-def err_template_arg_member_ptr_base_derived_not_supported : Error<
- "sorry, non-type template argument of pointer-to-member type %1 that refers "
- "to member %q0 of a different class is not supported yet">;
def err_template_arg_invalid : Error<
"non-type template argument '%0' is invalid">;
def ext_template_arg_extra_parens : ExtWarn<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index cf2d4fbe6d3ba1..a3881709263daa 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8569,8 +8569,8 @@ class Sema final {
QualType ParamType,
SourceLocation Loc);
ExprResult
- BuildExpressionFromIntegralTemplateArgument(const TemplateArgument &Arg,
- SourceLocation Loc);
+ BuildExpressionFromNonTypeTemplateArgument(const TemplateArgument &Arg,
+ SourceLocation Loc);
/// Enumeration describing how template parameter lists are compared
/// for equality.
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index d9cefcaa84d7e5..2df986c8541057 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -6753,6 +6753,11 @@ ASTContext::getCanonicalTemplateArgument(const TemplateArgument &Arg) const {
case TemplateArgument::Integral:
return TemplateArgument(Arg, getCanonicalType(Arg.getIntegralType()));
+ case TemplateArgument::StructuralValue:
+ return TemplateArgument(*this,
+ getCanonicalType(Arg.getStructuralValueType()),
+ Arg.getAsStructuralValue());
+
case TemplateArgument::Type:
return TemplateArgument(getCanonicalType(Arg.getAsType()),
/*isNullPtr*/ false, Arg.getIsDefaulted());
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index b762d6a4cd3800..8c279dd1eb048b 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -823,6 +823,17 @@ ASTNodeImporter::import(const TemplateArgument &From) {
From.getIsDefaulted());
}
+ case TemplateArgument::StructuralValue: {
+ ExpectedType ToTypeOrErr = import(From.getStructuralValueType());
+ if (!ToTypeOrErr)
+ return ToTypeOrErr.takeError();
+ Expected<APValue> ToValueOrErr = import(From.getAsStructuralValue());
+ if (!ToValueOrErr)
+ return ToValueOrErr.takeError();
+ return TemplateArgument(Importer.getToContext(), *ToTypeOrErr,
+ *ToValueOrErr);
+ }
+
case TemplateArgument::Template: {
Expected<TemplateName> ToTemplateOrErr = import(From.getAsTemplate());
if (!ToTemplateOrErr)
@@ -3572,6 +3583,8 @@ class IsTypeDeclaredInsideVisitor
case TemplateArgument::NullPtr:
// FIXME: The type is not allowed to be in the function?
return CheckType(Arg.getNullPtrType());
+ case TemplateArgument::StructuralValue:
+ return CheckType(Arg.getStructuralValueType());
case TemplateArgument::Pack:
for (const auto &PackArg : Arg.getPackAsArray())
if (checkTemplateArgument(PackArg))
diff --git a/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp
index a9e0d1698a9178..39a2f7ce61e135 100644
--- a/clang/lib/AST/ASTStructuralEquivalence.cpp
+++ b/clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -628,6 +628,10 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
return IsStructurallyEquivalent(Context, Arg1.getAsExpr(),
Arg2.getAsExpr());
+ case TemplateArgument::StructuralValue:
+ // FIXME: Do we need to customize the comparison?
+ return Arg1.structurallyEquals(Arg2);
+
case TemplateArgument::Pack:
return IsStructurallyEquivalent(Context, Arg1.pack_elements(),
Arg2.pack_elements());
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index e1440e5183a4e6..a2f50ae4ce8eeb 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -343,6 +343,10 @@ LinkageComputer::getLVForTemplateArgumentList(ArrayRef<TemplateArgument> Args,
LV.merge(getTypeLinkageAndVisibility(Arg.getNullPtrType()));
continue;
+ case TemplateArgument::StructuralValue:
+ LV.merge(getLVForValue(Arg.getAsStructuralValue(), computation));
+ continue;
+
case TemplateArgument::Template:
case TemplateArgument::TemplateExpansion:
if (TemplateDecl *Template =
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index b1678479888eb7..d3ef822b371d5e 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -4833,9 +4833,26 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
E = cast<CXXStdInitializerListExpr>(E)->getSubExpr();
goto recurse;
- case Expr::SubstNonTypeTemplateParmExprClass:
+ case Expr::SubstNonTypeTemplateParmExprClass: {
+ // Mangle a substituted parameter the same way we mangle the template
+ // argument.
+ auto *SNTTPE = cast<SubstNonTypeTemplateParmExpr>(E);
+ if (auto *CE = dyn_cast<ConstantExpr>(SNTTPE->getReplacement())) {
+ // Pull out the constant value and mangle it as a template argument.
+ QualType ParamType = SNTTPE->getParameterType(Context.getASTContext());
+ if (CE->hasAPValueResult())
+ mangleValueInTemplateArg(ParamType, CE->getResultAsAPValue(), false,
+ /*NeedExactType=*/true);
+ else
+ mangleValueInTemplateArg(ParamType, CE->getAPValueResult(), false,
+ /*NeedExactType=*/true);
+ break;
+ }
+ // The remaining cases all happen to be substituted with expressions that
+ // mangle the same as a corresponding template argument anyway.
E = cast<SubstNonTypeTemplateParmExpr>(E)->getReplacement();
goto recurse;
+ }
case Expr::UserDefinedLiteralClass:
// We follow g++'s approach of mangling a UDL as a call to the literal
@@ -6064,6 +6081,11 @@ void CXXNameMangler::mangleTemplateArg(TemplateArgument A, bool NeedExactType) {
mangleNullPointer(A.getNullPtrType());
break;
}
+ case TemplateArgument::StructuralValue:
+ mangleValueInTemplateArg(A.getStructuralValueType(),
+ A.getAsStructuralValue(),
+ /*TopLevel=*/true, NeedExactType);
+ break;
case TemplateArgument::Pack: {
// <template-arg> ::= J <template-arg>* E
Out << 'J';
@@ -6472,7 +6494,20 @@ void CXXNameMangler::mangleValueInTemplateArg(QualType T, const APValue &V,
Out << "plcvPcad";
Kind = Offset;
} else {
- if (!V.getLValuePath().empty() || V.isLValueOnePastTheEnd()) {
+ // Clang 11 and before mangled ...
[truncated]
|
| @@ -25,7 +25,7 @@ template <int &> struct S {}; // #dr1801-S | |||
| S<i> V; // #dr1801-S-i | |||
| // cxx98-14-error@-1 {{non-type template argument does not refer to any declaration}} | |||
| // cxx98-14-note@#dr1801-S {{template parameter is declared here}} | |||
| // since-cxx17-error@#dr1801-S-i {{non-type template argument refers to subobject '.i'}} | |||
| // cxx17-error@#dr1801-S-i {{non-type template argument refers to subobject '.i'}} | |||
There was a problem hiding this comment.
Quoting P1907R1 wording changes:
For a non-type template-parameter of reference or pointer type <...> the reference or pointer value shall not refer to or be the address of (respectively):
a subobject (6.7.2),
<...>
a subobject (6.7.2) of one of the above.
My reading is that you indeed can reference subobjects in NTTP since 20, unless they fall into one of the narrow categories listed in that paragraph. The change you made is correct, unless P1907R1 was adopted as a DR.
Sorry I forgot to check current wording when I added the test.
There was a problem hiding this comment.
I agree that this restriction no longer exist in the current wording. @zygoloid
| @@ -5401,6 +5409,8 @@ std::string CGDebugInfo::GetName(const Decl *D, bool Qualified) const { | |||
| // feasible some day. | |||
| return TA.getAsIntegral().getBitWidth() <= 64 && | |||
| IsReconstitutableType(TA.getIntegralType()); | |||
| case TemplateArgument::StructuralValue: | |||
| return false; | |||
There was a problem hiding this comment.
@dwblaikie, could you verify please? Maybe, non-type template arguments of floating point types are reconstitutable though?
There was a problem hiding this comment.
I don't think this is blocking either way, at worse we have suboptimal debugging experience for a new feature
There was a problem hiding this comment.
Sorry for the delays in the debug info review for this - it is on my list :/
There was a problem hiding this comment.
I did finally take a look at this - I think the Clang side of things is fine - if anything improvements could be made to LLVM to be able to lower constants to DWARF for a wider variety of values.
eg: For the float example, the IR is:
!8 = !DITemplateValueParameter(name: "F", type: !9, value: float 1.000000e+00)
But the DWARF backend can't handle the float constant - the handling is in DwarfUnit::constructTemplateValueParameterDIE (in llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp) and currently only handles ConstantInt (for basic stuff - bools, ints, etc) and GlobalValue (for pointer non-type template parameters).
So for these new floats and such, the DWARF doesn't include the value, only the type.
GCC uses this encoding, for instance:
0x0000002b: DW_TAG_template_value_parameter [3] (0x0000001e)
DW_AT_name [DW_FORM_string] ("F")
DW_AT_type [DW_FORM_ref4] (cu + 0x004d => {0x0000004d} "float")
DW_AT_const_value [DW_FORM_block1] (<0x04> 00 00 80 3f )
There was a problem hiding this comment.
Thanks for clarifying! Probably, I'll come back to this some day to extend the backend, but I'm not sure.
| @@ -25,7 +25,7 @@ template <int &> struct S {}; // #dr1801-S | |||
| S<i> V; // #dr1801-S-i | |||
| // cxx98-14-error@-1 {{non-type template argument does not refer to any declaration}} | |||
| // cxx98-14-note@#dr1801-S {{template parameter is declared here}} | |||
| // since-cxx17-error@#dr1801-S-i {{non-type template argument refers to subobject '.i'}} | |||
| // cxx17-error@#dr1801-S-i {{non-type template argument refers to subobject '.i'}} | |||
There was a problem hiding this comment.
Quoting P1907R1 wording changes:
For a non-type template-parameter of reference or pointer type <...> the reference or pointer value shall not refer to or be the address of (respectively):
a subobject (6.7.2),
<...>
a subobject (6.7.2) of one of the above.
My reading is that you indeed can reference subobjects in NTTP since 20, unless they fall into one of the narrow categories listed in that paragraph. The change you made is correct, unless P1907R1 was adopted as a DR.
Sorry I forgot to check current wording when I added the test.
| @@ -12,6 +12,7 @@ | |||
| #include "clang/AST/DeclCXX.h" | |||
| #include "clang/AST/DeclTemplate.h" | |||
| #include "clang/AST/DeclVisitor.h" | |||
| #include "clang/AST/ODRHash.h" | |||
There was a problem hiding this comment.
ODRHash is used inside the newly added code. Btw, what do you mean by "necessary"? I think one should not rely on transitive #includes in general. (I'm just an active contributor of the IWYU project. Trust me!)
There was a problem hiding this comment.
Yep, this one looks that it's needed, sorry.
The idea (for the other comments), is that we try to avoid changes in PRs that are not strictly related to the change being made, mostly to avoid conflicts.
If we wanted to follow a IWYU style and reorganize our headers, we would prefer separate PRs or NFC changes (although if we wanted to sort that kind of policy, we would have to do an RFC - again, because that sort of refactor is likely to induce conflicts).
There was a problem hiding this comment.
Just to clarify: do you insist that #include set added in a PR should be minimal for compilability?
There was a problem hiding this comment.
We shouldn't be minimizing the #include set, but it is often a 'red flag' when someone adds one, so Corentin brought it up.
IMO, I have no problem with adding the include here, and a IWYU RFC would be welcomed (and I am less concerned about the 'induce conflicts' as these are touched rarely).
clang/test/Modules/odr_hash.cpp should cover AST serialization/deserialization. But I could missing something, of course. What is your concern? |
|
Btw, I have a local branch with a few distinct commits. I could temporarily push it to simplify review process, and then squash them before merging. |
Looking at those tests again, they seem sufficient. |
|
I'm tempted to think that we should accept / merge this now @AaronBallman The logic is that this is a pretty big hole in our C++20 support and I don't think it's reasonable to try a merge after the deadline for 18. WDYT? |
From our past experience with release managers, they seem quite generous with deadlines to merge stuff in, as long as maintainers agree. I think the real question is how much time do we want this to bake in tree before users get to see it. |
There was a problem hiding this comment.
I would love it if @efriedma-quic / @rjmccall looked at the mangling parts, and @dwblaikie looked at the Debug info.
Else, LGTM.
| @@ -551,6 +646,18 @@ static const T &DiagTemplateArg(const T &DB, const TemplateArgument &Arg) { | |||
| case TemplateArgument::Integral: | |||
| return DB << toString(Arg.getAsIntegral(), 10); | |||
|
|
|||
| case TemplateArgument::StructuralValue: { | |||
| // FIXME: We're guessing at LangOptions! | |||
There was a problem hiding this comment.
This is pretty unfortunate, but it looks like we're doing this in a few places.
| @@ -12,6 +12,7 @@ | |||
| #include "clang/AST/DeclCXX.h" | |||
| #include "clang/AST/DeclTemplate.h" | |||
| #include "clang/AST/DeclVisitor.h" | |||
| #include "clang/AST/ODRHash.h" | |||
There was a problem hiding this comment.
We shouldn't be minimizing the #include set, but it is often a 'red flag' when someone adds one, so Corentin brought it up.
IMO, I have no problem with adding the include here, and a IWYU RFC would be welcomed (and I am less concerned about the 'induce conflicts' as these are touched rarely).
There was a problem hiding this comment.
Ping @rjmccall for Itanium mangling expertise to make sure we're matching the spec.
There are a few merge conflicts that also cropped up which need to be resolved, but overall I think this is pretty close to ready to try to re-land. It would be nice if we could get this in before the Clang 18 branch next Tue if possible (no pressure though).
| @@ -5401,6 +5409,8 @@ std::string CGDebugInfo::GetName(const Decl *D, bool Qualified) const { | |||
| // feasible some day. | |||
| return TA.getAsIntegral().getBitWidth() <= 64 && | |||
| IsReconstitutableType(TA.getIntegralType()); | |||
| case TemplateArgument::StructuralValue: | |||
| return false; | |||
…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)
`OpaqueValueExpr` doesn't necessarily contain a source expression. Particularly, after llvm#78041, it is used to carry the type and the value kind of a non-type template argument of floating-point type or referring to a subobject (those are so called `StructuralValue` arguments). This fixes llvm#79575. (cherry picked from commit ef67f63)
I'm running a reduction, but it progresses extremely slowly. Will post as soon as it converges to something meaningful (or I'll get some time to reduce the test case by hand). |
…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)
Fixes a regression from llvm#78041 as reported in the review. The original patch failed to compare the canonical type, which this adds. A slightly modified test of the original report is added. (cherry picked from commit e3ee376)
`OpaqueValueExpr` doesn't necessarily contain a source expression. Particularly, after llvm#78041, it is used to carry the type and the value kind of a non-type template argument of floating-point type or referring to a subobject (those are so called `StructuralValue` arguments). This fixes llvm#79575. (cherry picked from commit ef67f63)
…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)
Fixes a regression from llvm#78041 as reported in the review. The original patch failed to compare the canonical type, which this adds. A slightly modified test of the original report is added. (cherry picked from commit e3ee376)
`OpaqueValueExpr` doesn't necessarily contain a source expression. Particularly, after llvm#78041, it is used to carry the type and the value kind of a non-type template argument of floating-point type or referring to a subobject (those are so called `StructuralValue` arguments). This fixes llvm#79575. (cherry picked from commit ef67f63)
…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)
Fixes a regression from llvm#78041 as reported in the review. The original patch failed to compare the canonical type, which this adds. A slightly modified test of the original report is added. (cherry picked from commit e3ee376)
`OpaqueValueExpr` doesn't necessarily contain a source expression. Particularly, after llvm#78041, it is used to carry the type and the value kind of a non-type template argument of floating-point type or referring to a subobject (those are so called `StructuralValue` arguments). This fixes llvm#79575. (cherry picked from commit ef67f63)
…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)
Fixes a regression from llvm#78041 as reported in the review. The original patch failed to compare the canonical type, which this adds. A slightly modified test of the original report is added. (cherry picked from commit e3ee376)
`OpaqueValueExpr` doesn't necessarily contain a source expression. Particularly, after llvm#78041, it is used to carry the type and the value kind of a non-type template argument of floating-point type or referring to a subobject (those are so called `StructuralValue` arguments). This fixes llvm#79575. (cherry picked from commit ef67f63)
…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)
Introduced in llvm#78041, originally reported as llvm#79957 and fixed partially in llvm#80050. `OpaqueValueExpr` used with `TemplateArgument::StructuralValue` has no corresponding source expression. A test case with subobject-referring NTTP added.
…he template differ This was not implemented in #78041 This patch does not implement this fucntionality, it just falls back to the expression when possible. Otherwise, such as when dealing with canonical types to begin with, this will just ignore the argument as if it wasn't even there. Fixes #93068
|
This missed adding support to StructuralValue template arguments to the template differ. See #93265 The support is still missing, we are just avoiding the crash for now. |
…he template differ This was not implemented in #78041 This patch does not implement this fucntionality, it just falls back to the expression when possible. Otherwise, such as when dealing with canonical types to begin with, this will just ignore the argument as if it wasn't even there. Fixes #93068
…he template differ (#93265) This was not implemented in #78041 when StructuralValue TemplateArguments were originally added. This patch does not implement this functionality, it just falls back to the expression when possible. Otherwise, such as when dealing with canonical types to begin with, this will just ignore the argument as if it wasn't even there. Fixes #93068
|
This crash is due to this set of changes: #112222 |
Previously committed as 9e08e51, and reverted because a dependency commit was reverted, then committed again as 4b57400 and reverted again because "dependency commit" 5a391d3 was reverted. But it doesn't seem that 5a391d3 was a real dependency for this.
This commit incorporates 4b57400 and 18e093f by Richard Smith (@zygoloid), with some minor fixes, most notably:
UncommonValuerenamed toStructuralValueVK_PRValueinstead ofVK_RValueas default kind in lvalue and member pointer handling branch inBuildExpressionFromNonTypeTemplateArgumentValue;handling of
StructuralValueinIsTypeDeclaredInsideVisitor;filling in
SugaredConvertedalong withCanonicalConvertedparameter inSema::CheckTemplateArgument;minor cleanup in
TemplateInstantiator::transformNonTypeTemplateParmRef;TemplateArgumentconstructors refactored;ODRHashcalculation forUncommonValue;USR generation for
UncommonValue;more correct MS compatibility mangling algorithm (tested on MSVC ver. 19.35; toolset ver. 143);
IR emitting fixed on using a subobject as a template argument when the corresponding template parameter is used in an lvalue context;
noundefattribute and opaque pointers intemplate-argumentstest;analysis for C++17 mode is turned off for templates in
warn-bool-conversiontest; in C++17 and C++20 mode, array reference used as a template argument of pointer type produces template argument of UncommonValue type, andBuildExpressionFromNonTypeTemplateArgumentValuemakesOpaqueValueExprfor it, andDiagnoseAlwaysNonNullPointercannot see through it; despite of "These cases should not warn" comment, I'm not sure about correct behavior; I'd expect a suggestion to replaceifbyif constexpr;temp.arg.nontype/p1.cppanddr18xx.cpptests fixed.