[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 1 09:18:49 PDT 2023
aaron.ballman added a comment.
In D140996#4655288 <https://reviews.llvm.org/D140996#4655288>, @bolshakov-a wrote:
> Sorry, but I don't know what remains to be done here. It seems that the only important question is about ABI, but I've already answered that the changes under discussion seem to be already fixed in the Itanium ABI document.
Oh gosh, this must have fallen through the cracks then -- I thought it was waiting on further changes, so I hadn't been re-reviewing it. I'm sorry about that! Let's get this ball rolling again to try to get this landed. CC @erichkeane (who may not be available for the next while due to WG21 meetings, FYI).
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2205-2206
"bit-field%select{| %1}2">;
+def err_reference_bind_to_bitfield_in_cce : Error<
+ "reference cannot bind to bit-field in converted constant expression">;
def err_reference_bind_to_vector_element : Error<
----------------
This change seems somewhat orthogonal to the rest of the patch; should this be split out? Also, there doesn't appear to be test coverage for the new diagnostic.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4397
+ // argument.
+ // As proposed in https://github.com/itanium-cxx-abi/cxx-abi/issues/111.
+ auto *SNTTPE = cast<SubstNonTypeTemplateParmExpr>(E);
----------------
bolshakov-a wrote:
> bolshakov-a wrote:
> > efriedma wrote:
> > > bolshakov-a wrote:
> > > > erichkeane wrote:
> > > > > erichkeane wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > We should get this nailed down. It was proposed in Nov 2020 and the issue is still open. CC @rjmccall
> > > > > > This definitely needs to happen. @rjmccall or @eli.friedman ^^ Any idea what the actual mangling should be?
> > > > > This is still an open, and we need @rjmccall @eli.friedman or @asl to help out here.
> > > > Ping @efriedma, @rjmccall, @asl.
> > > I'm not really familiar with the mangling implications for this particular construct, nor am I actively involved with the Itanium ABI specification, so I'm not sure how I can help you directly.
> > >
> > > That said, as a general opinion, I don't think it's worth waiting for updates to the Itanuim ABI document to be merged; such updates are happening slowly at the moment, and having a consistent mangling is clearly an improvement even if it's not specified. My suggested plan of action:
> > >
> > > - Make sure you're satisfied the proposed mangling doesn't have any holes you're concerned about (i.e. it produces a unique mangling for all the relevant cases). If you're not sure, I can try to spend some time understanding this, but it doesn't sound like you have any concerns about this.
> > > - Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
> > > - Go ahead with this.
> > > Put a note on the issue in the Itanium ABI repo that you're planning to go ahead with using this mangling in clang. Also send an email directly to @rjmccall and @rsmith in case they miss the notifications.
> >
> > I'm sorry for noting one more time that Richard already pushed these changes in clang upstream, but they had been just reverted.
> >
> > Maybe, I should make a PR into Itanium API repository, but I probably need some time to dig into the theory and all the discussions. But yes, even NTTP argument mangling rules are not still merged: https://github.com/itanium-cxx-abi/cxx-abi/pull/140
> @aaron.ballman, @erichkeane, seems like it is already fixed in the ABI document:
> > Typically, only references to function template parameters occurring within the dependent signature of the template are mangled this way. In other contexts, template instantiation replaces references to template parameters with the actual template arguments, and mangling should mangle such references exactly as if they were that template argument.
>
> https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.template-param
>
> See also [the discussion in the issue](https://github.com/itanium-cxx-abi/cxx-abi/issues/111#issuecomment-1567486892).
Okay, I think I agree that this is already addressed in the ABI document. I think we can drop the comment referencing the ABI issue, wdyt?
================
Comment at: clang/lib/AST/TemplateBase.cpp:408-409
case Integral:
- getAsIntegral().Profile(ID);
getIntegralType().Profile(ID);
+ getAsIntegral().Profile(ID);
+ break;
----------------
bolshakov-a wrote:
> aaron.ballman wrote:
> > Why did the order of these calls change?
> I don't know, it is from 9e08e51a20d0d2. I've tried to invert the order along with the order for `StructuralValue`, and all tests have been passed.
I don't think the order should matter -- `Profile()` does a hash combination, so this should be reasonable I suppose.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5849-5855
+ // TryCopyInitialization returns incorrect info for attempts to bind reference
+ // to bit-field due to C++ [over.ics.ref]p4, so check it here.
+ if (From->refersToBitField() && T.getTypePtr()->isReferenceType()) {
+ return S.Diag(From->getBeginLoc(),
+ diag::err_reference_bind_to_bitfield_in_cce)
+ << From->getSourceRange();
+ }
----------------
This is the code using the new diagnostic that I had questions about above; seems orthogonal and perhaps should be done separately, but I could be missing something too.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5983-5985
+ Expr *E = Result.get();
+ if (!isa<ConstantExpr>(E))
+ E = ConstantExpr::Create(S.Context, Result.get(), Value);
----------------
bolshakov-a wrote:
> aaron.ballman wrote:
> > I thought we could run into situations where we had a `ConstantExpr` but it did not yet have its result stored in it. Should we assert that isn't the case here?
> If I understand correctly, the sole place where `ConstantExpr` is constructed which may occur here is `BuildExpressionFromNonTypeTemplateArgumentValue` function, and a value is set into it there. Should I add the assertion into code?
Yeah, I think it might make sense to do:
```
if (isa<ConstantExpr>(E)) {
// We expect a ConstantExpr to have a value associated with it by this point.
assert(E->getResultStorageKind() != ConstantExpr::RSK_None && "ConstantExpr has no value associated with it");
} else {
E = ConstantExpr::Create(S.Context, Result.get(), Value);
}
```
(May have to adjust for formatting.)
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7968
+ case APValue::FixedPoint:
+ return FixedPointLiteral::CreateFromRawInt(
+ S.Context, Val.getFixedPoint().getValue(), T, Loc,
----------------
What should happen if `T` isn't a fixed point type? (Should we assert here?)
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8033
+
+ case TemplateArgument::NullPtr:
+ case TemplateArgument::Declaration:
----------------
I'm a bit surprised that nullptr is modeled as a declaration rather than an integral value; can you explain that a bit? (I do see that the existing code in `BuildExpressionFromDeclTemplateArgument()` does have a case for handling nullptr, so the changes may be fine as-is.)
I don't test coverage for use of `nullptr_t` as an NTTP, unless I've missed it.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7882
+ Sema &S, QualType T, const APValue &Val, SourceLocation Loc) {
+ auto MakeInitList = [&](ArrayRef<Expr *> Elts) -> Expr * {
+ auto *ILE = new (S.Context) InitListExpr(S.Context, Loc, Elts, Loc);
----------------
erichkeane wrote:
> I don't have a good idea of what is happening in this function here, it isn't really clear... before committing, someone needs to do a deep dive on this function for review.
I've reviewed it, and the logic makes sense to me. The basic gist is that we're given an APValue for the NTTP and we need to gin up an expression of the correct type which resolves to the same value.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:6303-6305
case TemplateArgument::NullPtr:
- MarkUsedTemplateParameters(Ctx, TemplateArg.getNullPtrType(), OnlyDeduced,
- Depth, Used);
----------------
Can you explain this change a bit?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140996/new/
https://reviews.llvm.org/D140996
More information about the cfe-commits
mailing list