Page MenuHomePhabricator

[Concepts] Diagnose when 'concept' is specified on a specialization
ClosedPublic

Authored by nwilson on Oct 1 2015, 11:01 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

nwilson updated this revision to Diff 36270.Oct 1 2015, 11:01 AM
nwilson retitled this revision from to [Concepts] Add diagnostic; specializations of variable and function concepts.
nwilson updated this object.
nwilson added a subscriber: cfe-commits.
test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.concept/p7.cpp
13 ↗(On Diff #36270)

"extern template" is a form of explicit instantiation as well. I suggest testing that for both the function concept and variable concept cases.

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.concept/p7.cpp
4 ↗(On Diff #36270)

This is not a declaration (never mind definition) of a function template or variable template, but of a specialization. Presumably, this violates [dcl.spec.concept]p1. Perhaps a test for p7 should omit the concept specifier? The same logic may apply to the explicit specialization cases.

rsmith added inline comments.Oct 7 2015, 2:59 PM
lib/Sema/SemaDecl.cpp
5902–5915 ↗(On Diff #36270)

Maybe combine these into a single check, and use << (IsPartialSpecialization ? 2 : 1) for the specialization kind.

7562 ↗(On Diff #36270)

Missing period.

7888 ↗(On Diff #36270)

I'm not convinced this is going to work: if the declaration is invalid because it's declared concept /and/ for another reason, we presumably want to use the same error recovery path as before.

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.concept/p7.cpp
1 ↗(On Diff #36270)

This file is in the wrong directory; the relevant section is [dcl.spec.concept], not [dcl.concept].

4 ↗(On Diff #36270)

Right, there are two different ways things can go wrong here:

  1. concept is specified on a non-template (such as in an explicit specialization or explicit instantiation declaration) or a variable template partial specialization
  2. an explicit or partial specialization or an explicit instantiation declares (*without* the concept keyword) a specialization of a template that was declared as a concept.

This patch is only checking for the former case, whereas the rule in [dcl.spec.concept]p7 says that the latter case is ill-formed. So these tests are at least in the wrong file. To check for violations of p7, you'll need to look at whether the template's pattern is marked as being a concept.

I'm also not sure where we say that a partial specialization of a variable template cannot be declared concept; that seems to be a defect in the TS wording.

nwilson updated this revision to Diff 37099.Oct 12 2015, 6:07 AM

Moving tests to the correct file.
Modifying the diagnostic id and message so it's more applicable to the checks in this Patch. Update the quoted sections of the standard as well.
Use ternary operator for Partial Specialization check.
Add missing period in comment.

nwilson retitled this revision from [Concepts] Add diagnostic; specializations of variable and function concepts to [Concepts] Diagnose when 'concept' is specified on a specialization.Oct 12 2015, 6:10 AM
nwilson marked 2 inline comments as done.Oct 12 2015, 6:33 AM
nwilson added inline comments.
lib/Sema/SemaDecl.cpp
7886 ↗(On Diff #37099)

Maybe there could be a problem further down if we think there aren't explicit template args and there really are. We could do a check similar to the isFriend check in the same block below like this:

else if (isConcept && isFunctionTemplateSpecialization) {

HasExplicitTemplateArgs = true;

}

But it seems like that's essentially the same thing.

nwilson updated this revision to Diff 37529.EditedOct 15 2015, 3:40 PM

Addressing Richard's other comment regarding the check that the FunctionDecl is not a concept. Also, not marking the specialization as invalid if it's declared with the concept specifier since the downstream parts of Clang should look at the primary template.

faisalv added inline comments.Oct 15 2015, 7:13 PM
include/clang/AST/Decl.h
1577 ↗(On Diff #37529)

My inclination would have been to add this bit to FunctionTemplateDecl, instead of to every FunctionDecl - since not every kind of FunctionDecl can be a concept ... My first instinct would have been to add an enum to TemplateKind, and then forward isConcept() to check if we are a template and if so, through it, if concept is specified?

But I suppose that adds more complexity, and you trade space for speed - For my own edification, could I ask you or Richard to comment on the cons of that approach - and why the current approach is preferred? (i.e. simplicity over complexity or space over time etc.)

nwilson added inline comments.Oct 22 2015, 9:09 PM
include/clang/AST/Decl.h
1577 ↗(On Diff #37529)

Sorry for the slow reply. Yeah, I thought it was a little more straightforward having isConcept as a member function of FunctionDecl (and the bit). It also seemed that we'd be okay in terms of size since 17 bits are already being used here and adding one more wouldn't go over a byte boundary. Maybe Richard has other thoughts though?

include/clang/AST/Decl.h
1577 ↗(On Diff #37529)

I am not sure what the original justification was for IsTrivial to be a bit here instead of in CXXMethodDecl, but it seems that similar considerations apply.

lib/Sema/SemaDecl.cpp
5909 ↗(On Diff #37529)

Should we mention that the partial specialization case is pending additional wording (under Concepts TS Issue 15)?

nwilson updated this revision to Diff 39030.Nov 2 2015, 8:36 PM

Updating to r251898

nwilson added inline comments.Nov 2 2015, 8:46 PM
lib/Sema/SemaDecl.cpp
5909 ↗(On Diff #39030)

Hmm, I'd lean toward leaving it as is until the wording is sorted out. I don't feel too strongly either way though.

nwilson updated this revision to Diff 40105.Nov 12 2015, 8:02 PM
  • Remove marking a variable concept invalid when specialized since we'll only look at the primary template downstream. This removal let's us use the same recovery path as before when 'concept' is specified on a non-template.

Comment added inline. Otherwise, LGTM.

lib/Sema/SemaDecl.cpp
7659 ↗(On Diff #40105)

I don't think the declaration should still be marked as a concept in this case.

aaron.ballman edited edge metadata.Nov 13 2015, 1:04 PM

Aside from one question, LGTM.

~Aaron

include/clang/Basic/DiagnosticSemaKinds.td
2003 ↗(On Diff #40105)

Is this an extraneous %?

nwilson added inline comments.Nov 18 2015, 3:10 PM
include/clang/Basic/DiagnosticSemaKinds.td
2003 ↗(On Diff #40105)

Good catch. I'll remove it when making the commit.

lib/Sema/SemaDecl.cpp
7659 ↗(On Diff #40105)

Hmm, Richard - did you have any thoughts about this? IIRC, we might be okay here by only looking at the concept flag of the primary template.

lib/Sema/SemaDecl.cpp
7659 ↗(On Diff #40105)

A consideration:
When processing the body associated with the specialization, should the requirements for a function concept body be checked?

rsmith added inline comments.Nov 18 2015, 5:36 PM
lib/Sema/SemaDecl.cpp
7659 ↗(On Diff #40105)

I think the concept flag logically belongs on the template declaration rather than on the templated declaration; moving it there would make this question irrelevant =)

nwilson added inline comments.Nov 30 2015, 2:39 PM
lib/Sema/SemaDecl.cpp
7659 ↗(On Diff #40105)

Sorry for the slow reply.

Okay. That makes sense. So, we'd be okay putting the IsConcept bit (and the associated member functions) in TemplateDecl then?

You'd be okay with the extra byte used?

nwilson updated this revision to Diff 41441.Nov 30 2015, 2:59 PM
nwilson edited edge metadata.

Updating to r254337

nwilson updated this revision to Diff 43072.Dec 16 2015, 3:08 PM
  • Move IsConcept bit and associated member functions from FunctionDecl to FunctionTemplateDecl.
  • Set the IsConcept flag using getDescribedFunctionTemplate.
nwilson updated this revision to Diff 43469.Dec 22 2015, 12:01 PM

Updating to r256247

rsmith added inline comments.Dec 22 2015, 2:22 PM
include/clang/AST/DeclTemplate.h
836 ↗(On Diff #43469)

This might make more sense on TemplateDecl, since we also want this flag for VarTemplateDecls. In any case, please use some existing spare bit for this rather than making all FunctionTemplateDecls 8 bytes larger by putting it here.

986 ↗(On Diff #43469)

Do we need a setter for this? (Can it change after the decl is constructed?)

lib/Sema/SemaDecl.cpp
7681 ↗(On Diff #41441)

Yes, I think that's a reasonable way forward. You can change either TemplatedDecl or TemplateParams into a PointerIntPair to avoid making TemplateDecl larger.

lib/Sema/SemaTemplate.cpp
7672–7674 ↗(On Diff #43469)

I don't think we need to quote the second sentence here.

7677 ↗(On Diff #43469)

No space after ::

faisalv added inline comments.
include/clang/AST/DeclTemplate.h
836 ↗(On Diff #43469)

@Nate - since you asked before, if I was to try and do this (now that we know Richard cares strongly about the extra space), I would probably risk complexity (and insult readability) by attempting to smuggle the extra bit into RedeclararableTemplateDecl::CommonBase::InstantiatedFromMember or perhaps within the low bits of the pointer to ReDeclarableTemplateDecl::CommonBase itself (i.e. Common)
Let's see if Richard has a preference - or suggests a completely different strategy for squirreling.

nwilson added inline comments.Dec 27 2015, 7:44 PM
include/clang/AST/DeclTemplate.h
836 ↗(On Diff #43469)

@rsmith - Yeah, I originally thought to put it in TemplateDecl but reconsidered since ClassTemplateDecls would get it as well . However, I guess by making TemplateDecl::TemplatedDecl an IntPointerPair that concern is irrelevant.

@faisalv - Thanks for the info and suggestion! This helped with Richard's suggestion (below). FYI - It looks like that IntPointerPair, InstantiatedFromMember, is for explicit specializations, and the pointer to CommonBase (Common) is null for a function concept's primary template declaration.

986 ↗(On Diff #43469)

The setter is used below when we see that concept is specified on the declaration. So, it wouldn't be initialized (or set) to true when the decl is constructed. Or, do you have a different opinion on where to set IsConcept to true such as how IsConstExpr is initialized in FunctionDecls being passed as a param in the constructor?

lib/Sema/SemaDecl.cpp
7727 ↗(On Diff #43469)

Thanks for the suggestion. I'll go with TemplatedDecl and see what you guys think.

nwilson updated this revision to Diff 43670.Dec 27 2015, 8:25 PM
  • Store the IsConcept boolean flag in TemplateDecl by making TemplatedDecl an IntPointerPair, and move the associated member functions into TemplateDecl.
  • Remove unnecessary quoted comment.
  • Remove an extra space where the diagnostic is used.

Ping. Now that the holidays are over-ish, as Aaron said in one of his Patches.

include/clang/AST/DeclTemplate.h
373 ↗(On Diff #43670)

This code is not limited to function concepts.

375 ↗(On Diff #43670)

The parameter is now unused and should be removed.

nwilson added inline comments.Jan 13 2016, 4:41 PM
include/clang/AST/DeclTemplate.h
375 ↗(On Diff #43670)

Hmm, I think I should actually use that parameter since it's a bug as is (and would still be if I removed the parameter). I'll plan on doing that unless there is another thought about this.

include/clang/AST/DeclTemplate.h
375 ↗(On Diff #43670)

My interpretation of http://reviews.llvm.org/D13357?id=43469#inline-129592 is that the function should take no parameters (there is no need for a way to set the property to false). Perhaps I misunderstood Richard's intent.

nwilson added inline comments.Jan 13 2016, 5:18 PM
include/clang/AST/DeclTemplate.h
377 ↗(On Diff #43469)

I can't seem to follow the link properly, but I'm assuming it's supposed to be where Richard is asking about whether we need a setter. I thought he meant whether we needed the setter function at all. Perhaps I misunderstood though.

But it seems to be the convention to give a boolean (or some other type of param) to a setter. I'd be fine either way though because as you said, there isn't a need to set the property to false. As a point of reference RedeclarableTemplateDecl::setMemberSpecialization doesn't take any params either.

Ping.

@rsmith - would you also mind clarifying the comment regarding setConcept(bool IC) at to whether it should exist at all or simply not have any params?

nwilson updated this revision to Diff 46588.Feb 1 2016, 4:10 PM
  • Fix a couple of comments to reflect the Patch.
  • Clang-format the changes in this Patch.
nwilson marked 5 inline comments as done.Feb 1 2016, 4:16 PM

Marking some comments Done which were fixed in previous updates.

rsmith added inline comments.Feb 1 2016, 5:57 PM
include/clang/AST/DeclTemplate.h
374 ↗(On Diff #46588)

I would prefer to not have a setter at all, but if it's awkward to pass this flag into the constructor, then a setter is fine (and I don't mind whether or not it takes a parameter). We should definitely not take a parameter and ignore it though.

nwilson added inline comments.Feb 1 2016, 7:12 PM
include/clang/AST/DeclTemplate.h
374 ↗(On Diff #46588)

Ah, gotcha. Yeah, it seemed somewhat awkward to me to pass the flag all the way through. As a note it would be VarTemplateDecl -> RedeclarableTemplateDecl -> TemplateDecl. Also, RedeclarableTemplateDecl also currently currently takes 7 parameters.

If you feel strongly, I can move it to the constructor. Otherwise, I'll take the parameter out of the setter as both you and Hubert have pointed out the property will not need to set to false.

nwilson updated this revision to Diff 46854.Feb 3 2016, 5:15 PM
  • This update removes the parameter in TemplateDecl::setConcept since there isn't a need to mark a concept false.

Minor comments; otherwise, LGTM.

lib/Sema/SemaDecl.cpp
6007 ↗(On Diff #46854)

We do not need to quote the second sentence here. The ellipsis in the first sentence should be expanded since the full list is necessary to conclude that the cases being diagnosed here are excluded.

7754 ↗(On Diff #46854)

We do not need to quote the second sentence here. The ellipsis in the first sentence should be expanded since the full list is necessary to conclude that the cases being diagnosed here are excluded.

nwilson updated this revision to Diff 46863.Feb 3 2016, 8:03 PM
  • Address Hubert's comments about the quoted section of the TS.
rsmith accepted this revision.Feb 4 2016, 1:06 PM
rsmith edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 4 2016, 1:06 PM
This revision was automatically updated to reflect the committed changes.