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

[bolshakov-a]

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.

[bolshakov-a]

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)

[aaron.ballman]

In D140996#4150848, @bolshakov-a wrote:

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]

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]

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]

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]

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.

[royjacobson]

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

[erichkeane]

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]

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

1675

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

1035

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.

[bolshakov-a]

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.

[bolshakov-a]

Rebased.

[bolshakov-a]

Fix after rebase.

[bolshakov-a]

Fix after rebase.

[bolshakov-a]

Refactor TemplateArgument constructors.

[bolshakov-a]

Add ODRHash calculation for UncommonValue (and test it).

[bolshakov-a]

Add ODRHash calculation for UncommonValue (and test it).

[bolshakov-a]

Add some testcases.

[bolshakov-a]

Fix constness issue in TemplateArgument for ASTContext.

[bolshakov-a]

Add relnote and update C++ status.

[bolshakov-a]

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

In D140996#4185795, @bolshakov-a wrote:

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

[bolshakov-a]

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

[bolshakov-a]

Fix formatting.

[erichkeane]

Patch seems to be missing all the context.

[bolshakov-a]

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?

[erichkeane]

In D140996#4189452, @bolshakov-a wrote:

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]

In D140996#4189452, @bolshakov-a wrote:

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.

[bolshakov-a]

Update patch with more context.

[bolshakov-a]

It works, thanks!

[erichkeane]

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

4503

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

clang/lib/AST/MicrosoftMangle.cpp

1675

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

1035

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

7944

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

1067

[akyrtzi]

clang/lib/Index/USRGeneration.cpp

1035

Adding @bnbarham to review the Index changes.

[bnbarham]

clang/lib/Index/USRGeneration.cpp

1035

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]

clang/lib/Index/USRGeneration.cpp

1035

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

[bnbarham]

clang/lib/Index/USRGeneration.cpp

1035

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]

clang/lib/Index/USRGeneration.cpp

1035

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]

clang/lib/Index/USRGeneration.cpp

1035

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]

clang/lib/AST/MicrosoftMangle.cpp

1675

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

1035

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.

[bolshakov-a]

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

[bolshakov-a]

Add USR generation and a couple of minor fixes.

[bolshakov-a]

Finalize renaming.

[bolshakov-a]

Finalize renaming one more time.

[erichkeane]

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

4503

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