Page MenuHomePhabricator

[Concepts] Constrained partial specializations and function overloads.
Needs ReviewPublic

Authored by saar.raz on Jan 10 2018, 10:43 AM.

Details

Summary

Added support for constraint satisfaction checking and partial ordering of constraints in constrained partial specialization and function template overloads. Depends on D41569.

Diff Detail

Event Timeline

saar.raz created this revision.Jan 10 2018, 10:43 AM
saar.raz updated this revision to Diff 137908.Mar 10 2018, 4:04 AM
  • Correct handling of non-substitutable concept specialization expressions.
saar.raz updated this revision to Diff 147187.May 16 2018, 2:54 PM
  • Adjusted constraint normalization to piecewise constraint substitution
  • Normalized constraints are now represented as described in the standard (a non-substituted expression along with a list of "template arguments").
saar.raz updated this revision to Diff 159323.Aug 6 2018, 9:49 AM

Adjusted to switch to ASTTemplateArgumentListInfo

saar.raz updated this revision to Diff 159331.Aug 6 2018, 10:24 AM
  • Fix bad handling of checking of deduced arguments in function templates
saar.raz updated this revision to Diff 159759.Aug 8 2018, 11:08 AM
  • Moved constraint checks to the end of FinishTemplateArgumentDeduction
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 4:32 PM
saar.raz updated this revision to Diff 195010.Apr 13 2019, 4:37 AM

Fixed bug in normalization substitution into CSEs
Rebase onto trunk

martong requested changes to this revision.Apr 16 2019, 1:34 AM
martong added a reviewer: balazske.
martong added inline comments.
lib/AST/ASTImporter.cpp
5034

ClassTemplate is in the "to" context, i.e. it is already imported.
PartialSpec is in the "from" context, not yet imported.
Thus, findPartialSpecialization is going search for a specialization in the "to" context but with constraints which are in the "from" context. So I suppose this will not find any match.
So, first you have to import the associated constraints expression and then use that in the search.

Something like:

ExpectedExpr ConstraintsOrErr = import(D->getAssociatedConstraints());
if (!ConstraintsOrErr)
  return ConstraintsOrErr.takeError();
// ...
ClassTemplate->findPartialSpecialization(TemplateArgs,
          *ConstraintsOrErr, InsertPos);

(The same below.)

This revision now requires changes to proceed.Apr 16 2019, 1:34 AM
balazske added inline comments.Apr 16 2019, 6:32 AM
lib/AST/ASTImporter.cpp
5105

This looks correct: PartSpec is the new instance (but this is not the best name for it, maybe PartialSpec2?).
But: How are the "associated constraint" expressions imported (stored somewhere in the To context)?

saar.raz updated this revision to Diff 196106.Apr 22 2019, 11:46 AM
  • Fixed importing of ACs when importing partial specs.
  • Adjust to getAssociatedConstraints interface change
saar.raz updated this revision to Diff 196107.Apr 22 2019, 11:51 AM

Renamed PartSpec to PartSpec2 (CR)

martong accepted this revision.Apr 23 2019, 7:50 AM

From the ASTImporter point of view it looks good to me, thanks for the changes!

This revision is now accepted and ready to land.Apr 23 2019, 7:50 AM
martong resigned from this revision.Apr 23 2019, 7:50 AM
This revision now requires review to proceed.Apr 23 2019, 7:50 AM
saar.raz marked an inline comment as done.Oct 14 2019, 2:28 PM
rsmith added a comment.Fri, Nov 1, 4:04 PM

(Didn't finish the review, but I have to run now.)

include/clang/AST/DeclTemplate.h
742

ProfileArguments &&...ProfileArgs?

1989–2004

Hmm, is this the right set of things to be profiling? The relevant rule should be that the template-heads are equivalent and the simple-template-ids are equivalent, and checking the associated constraints isn't sufficient to check the former. Should we be profiling the template parameter list and template arguments instead?

include/clang/Basic/DiagnosticSemaKinds.td
2469–2470

Please don't join sentences with periods like this; it looks strange given that we generally render diagnostics in lowercase, and not as full sentences). I'm also not sure what the second sentence here is telling the user. (What do you mean by "any"? "some", or "all", or substitutions the program requires, or something else?)

We generally use "while" not "when" for these "here's what I was doing" notes.

lib/Sema/SemaConcept.cpp
484–486

The specification says we substitute into the parameter mapping bottom-up (starting with atomic constraints and then substituting as they get used in enclosing constraints), but you're substituting top-down. That's not equivalent: specifically, the rules say that we should not perform substitution for parameters that are not used in the expansion of a concept ([temp.constr.atomic]p1: "[...] a mapping from the template parameters that appear within E to template arguments involving the [...]"). Example:

template<typename T> concept True = true;
template<typename T> concept Foo = True<T*>;
template<typename T> concept Bar = Foo<T&>;
template<typename T> void f() requires Bar<T>;

Here, the true atomic constraint has no parameter mappings, so we should never perform the substitution that would form T&*, and so shouldn't reject this for forming a pointer to a reference.

I think it's sufficient to first determine which parameters are used in a concept (perhaps when the concept is defined), and then form a parameter mapping top-down skipping the unused arguments, but you need to be careful to do that recursively (Bar doesn't use its T because Foo doesn't use its T).

777–778

There are a bunch of ways we can optimize this, but let's get the straightforward approach landed first :)

lib/Sema/SemaTemplate.cpp
3772

&& and || on prior line, please.