bolshakov-a added inline comments.
================
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<
----------------
aaron.ballman wrote:
> 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.
Separated into https://github.com/llvm/llvm-project/pull/71077
The problem showed up after one of rebasings when C++20 mode was turned on for
`CXX/drs/dr12xx.cpp` test in [that
commit](https://github.com/llvm/llvm-project/commit/653a82e95257a7cd3f22c24e40d54459a6608429).
================
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);
----------------
aaron.ballman wrote:
> 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?
Ok, dropped.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:7968
+ case APValue::FixedPoint:
+ return FixedPointLiteral::CreateFromRawInt(
+ S.Context, Val.getFixedPoint().getValue(), T, Loc,
----------------
aaron.ballman wrote:
> What should happen if `T` isn't a fixed point type? (Should we assert here?)
I don't expect that it will happen. Non-type template argument is a constant
expression converted to the type of the template parameter. Hence, a value
produced by such an expression should correspond to the NTTP type.
Should an assert be placed here, it should be placed in the other `switch`
branches as well, I think. The problem is with
`BuildExpressionFromNonTypeTemplateArgumentValue` interface: the parameters `T`
and `Val` of this function aren't fully independent from each other (likewise
`Type` and `Value` fields of the `TemplateArgument::V` structure). I'm not sure
whether it is worth to be fixed here somehow.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8033
+
+ case TemplateArgument::NullPtr:
+ case TemplateArgument::Declaration:
----------------
aaron.ballman wrote:
> 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.
`TemplateArgument::NullPtr` represents usually a `nullptr` given for a template
parameter of pointer-to-object or pointer-to-member type, i.e. a parameter
referring in other cases to a declaration of some object with static storage
duration. For this reason, it can be considered as a special case of
declaration-referring template argument. `nullptr` can also be an argument for
NTTP of `std::nullptr_t` type, but I have no idea when it is worth to use such
a parameter in real code. (Again, this change is from [the original Richard's
commit](https://github.com/llvm/llvm-project/commit/4b574008aef5a7235c1f894ab065fe300d26e786).
I'm just guessing what he had in mind.)
`temp_arg_nontype_cxx11.cpp`, `temp_arg_nontype_cxx1z.cpp`, and
`CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp` have already some cases of using
null pointers as template arguments, e.g.
[here](https://github.com/llvm/llvm-project/blob/llvmorg-18-init/clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp#L25)
and
[here](https://github.com/llvm/llvm-project/blob/llvmorg-18-init/clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp#L59).
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:6303-6305
case TemplateArgument::NullPtr:
- MarkUsedTemplateParameters(Ctx, TemplateArg.getNullPtrType(), OnlyDeduced,
- Depth, Used);
----------------
aaron.ballman wrote:
> Can you explain this change a bit?
I think the idea is that handling `TemplateArgument::NullPtr` case here is just
useless. This function is used in the process of determination which template
parameters of a templated function, deduction guide, or partial template
specialization could be deduced. Such parameters are searched in
parameter-declaration-clause, or in the template-argument-list of the
simple-template-id of a partial template specialization. But when a NTTP is
used as an argument of another template, that argument has the
`TemplateArgument::Expression` kind and stores an expression referring to the
NTTP. E.g., given such a template and its partial specialization:
```
template <int K, int N>
struct Templated {};
template <int M>
struct Templated<1, M> {};
```
the argument `1` has the `TemplateArgument::Integral` but is irrelevant to the
`M` value deduction, whereas the subsequent argument `M` has the
`TemplateArgument::Expression` kind. The `nullptr` case is similar.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140996/new/
https://reviews.llvm.org/D140996
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
