This is an archive of the discontinued LLVM Phabricator instance.

[c++20] P1907R1: Support for generalized non-type template arguments of scalar type.
Needs ReviewPublic

Authored by bolshakov-a on Jan 4 2023, 8:33 AM.

Details

Reviewers
aaron.ballman
rsmith
shafik
erichkeane
royjacobson
Group Reviewers
Restricted Project
Summary

Previously committed as 9e08e51a20d0d2b1c5724bb17e969d036fced4cd, and
reverted because a dependency commit was reverted, then commited again
as 4b574008aef5a7235c1f894ab065fe300d26e786 and reverted again because
"dependency commit" 5a391d38ac6c561ba908334d427f26124ed9132e was
reverted. But it doesn't seem that 5a391d38ac6c was a real dependency
for this.

This commit incorporates 4b574008aef5a7235c1f894ab065fe300d26e786 and
18e093faf726d15f210ab4917142beec51848258 by Richard Smith, with some
minor fixes, most notably:

  • VK_PRValue instead of VK_RValue as default kind in lvalue and

member pointer handling branch
in BuildExpressionFromNonTypeTemplateArgumentValue;

  • handling of UncommonValue in IsTypeDeclaredInsideVisitor;
  • filling in SugaredConverted along with CanonicalConverted

parameter in Sema::CheckTemplateArgument;

  • minor cleanup

in TemplateInstantiator::transformNonTypeTemplateParmRef;

  • noundef attribute and opaque pointers in template-arguments test;
  • analysis for C++17 mode is turned off for templates

in warn-bool-conversion test;
in C++17 and C++20 mode, array reference used as a template argument
of pointer type produces template argument of UncommonValue type, and
BuildExpressionFromNonTypeTemplateArgumentValue makes
OpaqueValueExpr for it, and DiagnoseAlwaysNonNullPointer cannot see
through it; despite of "These cases should not warn" comment, I'm not
sure about correct behavior; I'd expect a suggestion to replace if by
if constexpr;

  • temp.arg.nontype/p1.cpp test fixed.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@aaron.ballman, @rsmith, @mizvekov, @shafik, has the mankind any chance to get this reviewed and merged?

Sorry for the delay in review! I've changed the reviewer list a bit to get more visibility on this. Also, don't forget to add a release note for the changes. Should this also update the status in clang/www/cxx_status.html?

clang/lib/AST/ItaniumMangle.cpp
4594

We should get this nailed down. It was proposed in Nov 2020 and the issue is still open. CC @rjmccall

clang/lib/AST/TemplateBase.cpp
223–230

Well this is certainly a unique approach... Personally, I think it'd be nice to not assign to this within a constructor by calling other constructor; that falls squarely into "wait, what, can you even DO that?" kind of questions for me.

649

It's probably okay enough, but this is now the third instance of adding the same bug to this helper method -- maybe we should fix that instead?

clang/lib/Index/USRGeneration.cpp
1040

Any particular reason this isn't being handled now?

erichkeane added inline comments.Feb 24 2023, 6:57 AM
clang/include/clang/AST/ASTContext.h
3036 ↗(On Diff #486304)

This change is weird... what is going on here?

clang/include/clang/AST/TemplateBase.h
88

I definitely hate the name here... Just Value makes a bit more sense, but isn't perfectly accurate. Perhaps NonTypeValue? But that is a little redundant. Uncommon here is just strange and not particularly descriptive.

clang/lib/AST/MicrosoftMangle.cpp
1734

Has microsoft implemented this yet? Can we see what they chose to do and make sure we match them?

clang/lib/AST/ODRHash.cpp
182

I feel like this DEFINITELY needs to happen. Nullptr/integral is less important, but now that we have an actual value here, I think it needs to be part of the hash.

clang/lib/AST/TemplateBase.cpp
223–230

I agree, this function needs to be refactored to call some sort of 'init' function or something, even if we have to refactor the other constructors. This assignment to *this is just too strange to let stay.

649

Agreed, seems to me we should do the work NOW to just wire in the lang-opts to this function.

clang/lib/Sema/SemaTemplate.cpp
7943

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.

Thanks for the review! I definitely can try to answer questions and fix issues, but I just want to note that I'm not the original author, and these changes already were upstream. (Maybe, @rsmith will find some time for taking a look at this, after all?)

It would be nice if @mizvekov would check my changes regarding SugaredConverted vs CanonicalConverted.

Should this also update the status in clang/www/cxx_status.html?

I'm not sure. There still remains an issue with template parameters of reference type, which was tried to be fixed in 5a391d38ac6c. (On the other hand, MSVC team claims that their compiler supports P1907R1, despite that it simply crashes on some of the test cases from 5a391d38ac6c)

Should this also update the status in clang/www/cxx_status.html?

I'm not sure. There still remains an issue with template parameters of reference type, which was tried to be fixed in 5a391d38ac6c. (On the other hand, MSVC team claims that their compiler supports P1907R1, despite that it simply crashes on some of the test cases from 5a391d38ac6c)

Hmmm, okay, then I'd say we probably should update cxx_status. We can leave the status as "partial" but we can at least add details about what's still missing before we can claim full support. Kind of like we do for P0848R3 or P0315R4 on that page.

shafik added inline comments.Feb 24 2023, 2:53 PM
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
204

Shouldn't this be an error b/c it is a temporary? What is (int*)1 a subobject of?

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
21

using IPn = IntPtr<&n + 2> should be an error since that would be out of bounds, while +1 is ok b/c it is one past the end as long as we don't deref.

21

gcc reject this one but I think their pointer math is wonky here: https://godbolt.org/z/fhMqPPefG

25–31

We should reject IntPtr<&s.n[5]>; again b/c it is out of bounds.

shafik added inline comments.Feb 24 2023, 3:27 PM
clang/test/CodeGenCXX/template-arguments.cpp
5

I don't see any tests covering unions or enums.

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
12

I believe this is IFNDR the template-heads are functionally equivelent but not equivelent: https://eel.is/c++draft/temp.class#general-3

60

Can we add an enum example e.g.:

enum E{ E1, E2};
template <E> struct SE {};
using IPE = SE<E1>;
using IPE = SE<E2>;
shafik added inline comments.Feb 24 2023, 3:46 PM
clang/include/clang/AST/TemplateBase.h
88

This catch all UncommonValue feels artificial. Maybe we need a separate out the cases into more granular cases like Float etc....

clang/lib/AST/TemplateBase.cpp
223–230

I am going to third this sentiment, this is definitely not the right approach and the fact that you have this ad-hoc case below here also speaks to this.

bolshakov-a added inline comments.Feb 25 2023, 7:18 AM
clang/include/clang/AST/ASTContext.h
3036 ↗(On Diff #486304)

Here is not very beautiful attempt to workaround const-ness of TemplateArgument::V::Value pointer passed here from the added TemplateArgument constructor. The change in this line isn't acually needed and made only for consistence with the next line, I think. Alternatively, I can

  1. refactor addDestruction and AddDeallocation to work with pointers to constants, or
  2. add const_cast to AddDeallocation call in the next line, or
  3. make TemplateArgument::V::Value pointer non-const.

I'm biased to the first variant.

There are no AST [de]serialization tests in this PR, right? Would be nice to add some.

erichkeane added inline comments.Feb 27 2023, 5:55 AM
clang/include/clang/AST/ASTContext.h
3036 ↗(On Diff #486304)

I'd lean towards #3, it ends up being consistent with the rest of the things here. #1 is interesting, but that results in these functions violating const-correctness.

bolshakov-a added inline comments.Mar 10 2023, 10:38 AM
clang/include/clang/AST/ASTContext.h
3036 ↗(On Diff #486304)

I understand that calling the destructor on a reference to const looks strange, but it is reasonable: even constants should be destroyed.

clang/include/clang/AST/TemplateBase.h
88

@erichkeane, it looks strange, I agree. Even just CommonValue sounds better for me (but my English is far from fluent). Maybe, ArbitraryValue?

@shafik, your suggestion would move this PR far enough from the original Richard's commit. And I'd prefer to merge Declaration, Integral, and NullPtr kinds into that is currently called UncommonValue rather than to repeat here various APValue kinds.

clang/lib/AST/MicrosoftMangle.cpp
1734

Experimentally, I've made me sure that MSVC produces the same mangled names for the cases provided in the mangle-ms-templates test as in the test. But there are problems with references to subobjects: some cases are explicitly unsupported in mangleTemplateArgValue, and some cases produce a magled name different from what MSVC does. Should it be fixed in this PR, or may be delayed?

clang/lib/AST/TemplateBase.cpp
649

The problem here is because this function is called from a stream insertion operator, and there isn't obviously any way to pass 3rd parameter into it without switching it into an ordinary function.

clang/lib/Index/USRGeneration.cpp
1040

I need some guidance here. Which characters are allowed in the USR? Could the mangling algorithm from CXXNameMangler::mangleValueInTemplateArg be moved into some common place and reused here?

clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
204

This PR doesn't change C++17 mode diagnostics. Btw, in C++20 mode, this is acceptable template argument.

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
12

I don't think so, because <1.0f> and <2.0f / 2> are template argument lists and not template heads. Non-type template arguments may be written as arbitrary expressions which are converted before determining template argument equivalence. Expression result values are important.

60

What for? Enumerators as non-type template arguments are allowed since C++98, AFAIK. And this test is about changes in C++20.

I have some problems with Arcanist... It tries to open a new PR instead of updating this one. Probably because I've re-cloned my local repository.

Fix after rebase.

Fix after rebase.

Refactor TemplateArgument constructors.

Add ODRHash calculation for UncommonValue (and test it).

Add ODRHash calculation for UncommonValue (and test it).

Add some testcases.

Fix constness issue in TemplateArgument for ASTContext.

Add relnote and update C++ status.

@royjacobson, I've added some test cases for using the new NTTP arguments in clang modules. It uses serialization, in principle. Or more specialized tests are still needed?

@royjacobson, I've added some test cases for using the new NTTP arguments in clang modules. It uses serialization, in principle. Or more specialized tests are still needed?

No, I think that's good. Thanks for adding them!

Rebase and replace "Clang 16" with "Clang 17" in cxx_status.html (Clang 16 RC has already been branched off, AFAIK).

Fix formatting.

Patch seems to be missing all the context.

Sorry! It's my first time using Phabricator. Maybe, the problem occurs because I've solved the issue with Arcanist just by means of copy-pasting patches into "Update Diff" Web GUI form. Maybe, I should reopen the PR?

Sorry! It's my first time using Phabricator. Maybe, the problem occurs because I've solved the issue with Arcanist just by means of copy-pasting patches into "Update Diff" Web GUI form. Maybe, I should reopen the PR?

When you generate your patch, you need to use -U999999 to make sure you get full context. I typically pipe it to a file (the git diff /git show HEAD), then use the upload file version.

Endill added a subscriber: Endill.Mar 13 2023, 8:45 AM

Sorry! It's my first time using Phabricator. Maybe, the problem occurs because I've solved the issue with Arcanist just by means of copy-pasting patches into "Update Diff" Web GUI form.

There's nothing wrong with copy-pasting into web GUI form. You just have to export the patch with the right options, e.g. git diff HEAD~1 -U999999 > mypatch.patch (more on this here).

Maybe, I should reopen the PR?

You can simply update the review with properly exported patch.

Update patch with more context.

It works, thanks!

erichkeane added inline comments.
clang/include/clang/AST/TemplateBase.h
88

I don't think splitting out the individual cases has all that much value, at least until we NEED it.

As far as a name, what about StructuralValue? P1907 calls the 'type' of that A 'structural type'? It isn't perfect, but it at least seems to be somewhat defensible with standards language.

clang/lib/AST/ItaniumMangle.cpp
4594

This definitely needs to happen. @rjmccall or @eli.friedman ^^ Any idea what the actual mangling should be?

clang/lib/AST/MicrosoftMangle.cpp
1734

We need to end up doing our best to match the microsoft mangling if at all possible, since they own the ABI. I DEFINITELY would want any followup patch to be promised for Clang17 (that is, we don't release Clang17 with this patch and NOT that patch), so I'd expect said patch to be available for review before this gets committed.

As far as whether it needs to happen in THIS patch, we can perhaps decide based on the severity of the break, if you can provide examples (or, if it is split into a separate patch, we can use the tests there).

clang/lib/AST/TemplateBase.cpp
244

Why can this not happen now?

clang/lib/Index/USRGeneration.cpp
1040

I have no idea what is valid here. BUT @akyrtzi and @gribozavr (or @gribozavr2 ?) seem to be the ones that touch these files the most?

clang/lib/Sema/SemaTemplate.cpp
8000

Rather than this, I'd prefer these all down to the llvm_unreachable below. If we find they are reachable, then we'd be expected to implement them at that point.

clang/www/cxx_status.html
640
akyrtzi added inline comments.
clang/lib/Index/USRGeneration.cpp
1040

Adding @bnbarham to review the Index changes.

bnbarham added inline comments.Mar 13 2023, 2:01 PM
clang/lib/Index/USRGeneration.cpp
1040

Just visiting the underlying type seems reasonable, ie. VisitType(Arg.getUncommonValueType());. If it needed to be differentiated between a TemplateArgument::Type you could add a prefix character (eg. U), but that doesn't seem needed to me.

bolshakov-a added inline comments.Mar 13 2023, 2:05 PM
clang/lib/Index/USRGeneration.cpp
1040

Doesn't the holded value be added so as to distinguish e.g. Tpl<1.5> from Tpl<2.5>?

bnbarham added inline comments.Mar 13 2023, 3:09 PM
clang/lib/Index/USRGeneration.cpp
1040

Ah I see, yeah, we would. And there's no USR generation for APValue currently, which I assume is why your original question came up.

In general a USR just wants to uniquely identify an entity across compilations and isn't as restricted as the mangled name. For basically everything but LValue it seems like you'd be fine to print the value (for eg. int, float, etc), visit the underlying type (array, vector), or the visit the underlying decl (struct, union, member pointer). That's almost true for LValue as well, just with the extra parts that are also added to the ODR hash.

Alternatively, you could also just print the hash from Profile with the same handling as ODR hash. Worst case we'd accidentally merge specializations, but if that's good enough for the ODR hash it's probably good enough here as well.

bolshakov-a added inline comments.Mar 13 2023, 3:24 PM
clang/lib/Index/USRGeneration.cpp
1040

it seems like you'd be fine to print the value (for eg. int, float, etc)

I'm in doubt about the dot inside a floating point value representation. Minus sign is allowed, as I can see for TemplateArgument::Integral case.

bnbarham added inline comments.Mar 13 2023, 3:28 PM
clang/lib/Index/USRGeneration.cpp
1040

As long as there's a prefix for APValue and its kind, the dot is fine (eg. maybe @AP@ and then f for float, i for integer, etc).

bolshakov-a added inline comments.Mar 19 2023, 2:11 PM
clang/lib/AST/MicrosoftMangle.cpp
1734

I've addressed some issues already present on the main branch in D146386. I could try to fix remaining issues in this PR afrer landing that one.

clang/lib/AST/TemplateBase.cpp
244

Adding const to the TemplateArgument::DA::D type and to the TemplateArgument::getAsDecl() return type would lead to many changes unrelated to this PR.

clang/lib/Index/USRGeneration.cpp
1040

Thank you! I've decided to go the simplest way, i. e. to use ODRHash here. Should I write a test case (or some test cases), or they could become fragile due to possible ODRHash implementation changes? I've checked USR locally a little.

Rename UncommonValue to StructuralValue and add "TODO" about it.

Add USR generation and a couple of minor fixes.

Finalize renaming.

Finalize renaming one more time.

erichkeane added a subscriber: asl.Mar 20 2023, 7:08 AM

I've got the 1 concern with the mangling that I REALLY want one of our codegen owners to chime in on, otherwise this LGTM.

clang/lib/AST/ItaniumMangle.cpp
4594

This is still an open, and we need @rjmccall @eli.friedman or @asl to help out here.

bnbarham added inline comments.Mar 21 2023, 2:25 PM
clang/lib/Index/USRGeneration.cpp
1040

You could add a test that checks the ref has the same USR as the def, but yeah, I wouldn't specifically check the USR here.

Add type to USR so as not to get confused with

template <auto*> struct S {};
struct { int n; } s;

template<>
struct S<(void*)&s.n> {};

template<>
struct S<&s.n> {};

Add test on structural type NTTP USR. Just a couple of cases have been added because most of the relevant code should already be covered by odr_hash.cpp test.

Rebased; somehow incorrectly merged ReleaseNotes.rst fixed.

CXX/drs/dr12xx.cpp starts to show that clang accepts now references to bitfields as template arguments in C++20 mode. It is probably not OK.

bolshakov-a added a subscriber: hubert.reinterpretcast.

Avoid binding references in template arguments to bit-fields. @erichkeane, @hubert.reinterpretcast, please verify.

Fix MS compatibility mangling algorithm. Tested with MSVC ver. 19.35 (toolset ver. 143).

bolshakov-a added inline comments.
clang/lib/AST/ItaniumMangle.cpp
4594
efriedma added inline comments.May 14 2023, 12:33 PM
clang/lib/AST/ItaniumMangle.cpp
4594

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.
4600

I'm not sure what the point of the if (CE->hasAPValueResult()) is; are you just trying to avoid copying the APValue? (If this is going to be a repeating pattern, maybe we can add some sort of utility class to represent the pattern.)

bolshakov-a added inline comments.May 14 2023, 1:08 PM
clang/lib/AST/ItaniumMangle.cpp
4600

I don't know too, it is Richard's code. Looks really like an optimization.

bolshakov-a added inline comments.May 14 2023, 2:04 PM
clang/lib/AST/ItaniumMangle.cpp
4594

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

bolshakov-a added inline comments.Jun 6 2023, 11:40 AM
clang/lib/AST/ItaniumMangle.cpp
4594

@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.

It would be great if we could make progress on this for clang 17, I was told it's forcing some people to use other compilers.
Are the recent changes to the itanium abi enough to complete this work? Thanks!

I've not spotted any major concerns with this patch, but I did have some minor nits to look into. I'd love to hear from @rsmith and @erichkeane before signing off on this, as the changes are pretty involved and they've both done some in-depth looks at the patch already.

clang/include/clang/AST/TemplateBase.h
168–173

Can you add parameter names for the unnamed ones (we typically only leave a parameter unnamed when it is unused).

382–385
clang/lib/AST/MicrosoftMangle.cpp
1621–1623

Does this work or is the const_cast actually required?

clang/lib/AST/ODRHash.cpp
1281–1284
1287
1298–1299

I think this might be a bit clearer, but I don't insist on the change.

clang/lib/AST/TemplateBase.cpp
163–181
408–415

Why did the order of these calls change?

649

Okay, that's reasonable enough to hold off on changing. Thanks!

clang/lib/Sema/SemaOverload.cpp
5990–5999

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?

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
60

Sometimes we're lacking coverage for existing features, so when updating code in the area, we'll sometimes ask for extra coverage just to be sure we're not regressing something we think might not have a lot of existing test coverage.

clang/www/cxx_status.html
638

Fixes after review.

Btw, formatting of unrelated lines has leaked into TemplateBase.h. Sorry.

clang/lib/AST/MicrosoftMangle.cpp
1621–1623

No, it doesn't compile, likewise the standard C++ dynamic_cast cannot remove constness.

clang/lib/AST/TemplateBase.cpp
408–415

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.

clang/lib/Sema/SemaOverload.cpp
5990–5999

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?

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
60

temp_arg_nontype.cpp test already has some enum cases. If a case with type alias should be added, it shoud be added there, not in the temp_arg_nontype_cxx20.cpp, I think.

bolshakov-a added inline comments.Aug 6 2023, 4:00 PM
clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
60

I've just realized that C++98 didn't had type aliases. But typedefs should probably go as well.

It would be nice to figure out a plan for this PR in the next few weeks, before we get kicked out of Phab!

It would be nice to figure out a plan for this PR in the next few weeks, before we get kicked out of Phab!

Agreed -- @bolshakov-a are you still planning to actively work on this patch (it's okay if you're not, but in that case, do you mind if someone commandeers the patch)? We've got a few weeks left until Phabricator is going into read-only mode, so it would be good if we could get this review across the finish line before mid-Nov if possible.

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.

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).

clang/include/clang/Basic/DiagnosticSemaKinds.td
2205–2206

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.

clang/lib/AST/ItaniumMangle.cpp
4594

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?

clang/lib/AST/TemplateBase.cpp
408–415

I don't think the order should matter -- Profile() does a hash combination, so this should be reasonable I suppose.

clang/lib/Sema/SemaOverload.cpp
5849–5855

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.

5990–5999

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.)

clang/lib/Sema/SemaTemplate.cpp
7943

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.

7968

What should happen if T isn't a fixed point type? (Should we assert here?)

8033

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.

clang/lib/Sema/SemaTemplateDeduction.cpp
6303–6305

Can you explain this change a bit?

bolshakov-a added inline comments.Nov 3 2023, 7:20 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2205–2206

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.

clang/lib/AST/ItaniumMangle.cpp
4594

Ok, dropped.

clang/lib/Sema/SemaTemplate.cpp
7968

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.

8033

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. 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 and here.

clang/lib/Sema/SemaTemplateDeduction.cpp
6303–6305

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.

Fixes after review.

One more problem has been discovered and fixed: IR emitting didn't work when using a subobject as a template argument when the corresponding template parameter is used in an lvalue context. A test case has been added into CodeGenCXX/template-arguments.cpp test (but, maybe, there is a more appropriate place?)