This is an archive of the discontinued LLVM Phabricator instance.

[CONCEPTS] Add diagnostics: non-defining function; non-namespace scope
ClosedPublic

Authored by nwilson on Jul 7 2015, 10:17 PM.

Details

Summary

Create diagnostic for function concept declaration which is not a
definition.

Create diagnostic for concept declaration which isn't in namespace
scope.

Create associated tests.

Diff Detail

Event Timeline

nwilson updated this revision to Diff 29241.Jul 7 2015, 10:17 PM
nwilson retitled this revision from to [CONCEPTS] Creating Diagnostics for ill-formed function concept declaration.
nwilson updated this object.
nwilson added a subscriber: cfe-commits.
include/clang/Basic/DiagnosticSemaKinds.td
1964

This error is similar to err_objc_decls_may_only_appear_in_global_scope below, so perhaps it could be expressed in a similar manner.

1966

Perhaps: "can only declare a function concept with its definition".

The existing wording seems to imply that an earlier definition (of just one somewhere) is sufficient and that function concepts may be redeclared.

lib/Sema/SemaDecl.cpp
7464

The quoted text is a definition of the term "concept definition". The diagnosable rule is established by the quote on line 7444. So there probably should be a comment here to the effect of "we are checking that the declaration is indeed a definition".

7471

Typo: s/concepts/concept/

test/SemaCXX/cxx-concept-declaration.cpp
4

I'm not sure why this is part of the current patch.

6

Same comment as on line 3.

nwilson added inline comments.Jul 8 2015, 9:45 PM
include/clang/Basic/DiagnosticSemaKinds.td
1964

Sure, change it to something similar.

1966

Makes sense. I'll change the wording.

lib/Sema/SemaDecl.cpp
7458

Any thoughts about moving this check? I think it can be moved above to inside HandleDeclarator before we look at acting on a function or variable declarator since this check is applicable to both a function and variable concept

Something like:
if (!D.getContext() == FileContext) {

Diag(...)

}

7464

That makes sense. I'll make the change.

test/SemaCXX/cxx-concept-declaration.cpp
6

I'll take them out since they're not necessary - I just wanted a couple of tests that would be pass, but yeah, it's unnecessary.

lib/Sema/SemaDecl.cpp
7458

You would need to use DC. The DeclContext is also adjusted "later" by adjustContextForLocalExternDecl in each of the function and variable cases. I'm not sure what happens with friend declarations. In this case, I think using DC before the adjustments makes sense (since we cannot declare a concept in block scope or as a friend).

nwilson updated this revision to Diff 29315.Jul 9 2015, 4:45 AM

Addressing comments -
Changing diagnostic name and update wording.
Move check for concept scope being in namespace scope to HandleDeclarator.
Update comment's wording
Remove some unnecessary tests.

include/clang/Basic/DiagnosticSemaKinds.td
1964

Perhaps s/function and variable concept/concept/; see Aaron's response for further changes.

lib/Sema/SemaDecl.cpp
4887

There seems to be an indenting issue caused by a tab character here.

7455

Since the quote from the relevant part of the Standard has been moved elsewhere, it makes sense to quote it again up to the "function template" part.

test/SemaCXX/cxx-concept-declaration.cpp
3

Aaron suggested that we have acceptance testing for concepts in a namespace scope other than the global one.

8

Since we now cover variable cases for the scope check as well, we should have a test for it.

nwilson added inline comments.Jul 9 2015, 7:18 AM
include/clang/Basic/DiagnosticSemaKinds.td
1964

to make mention of it here, Aaron suggested having, "concept declarations may only appear in namespace scope" (since there aren't any concept declarations other than function and variable ones).

Is there any other opinion or preference on the phrasing?

lib/Sema/SemaDecl.cpp
4887

Sorry. Will fix.

7455

That makes sense. I'll put it back in.

test/SemaCXX/cxx-concept-declaration.cpp
8

Will do.

include/clang/Basic/DiagnosticSemaKinds.td
1964

Apparently, Aaron and I are of the same mind on the phrasing. :)

nwilson updated this revision to Diff 29426.Jul 9 2015, 9:21 PM

Update phrasing for concept declaration scope diagnostic
Fix indentation issue
Update comments for function declaration being a definition
Adding acceptance tests for concepts in namespace scope and adding diagnostic test for variable concept not being in namespace scope

nwilson added inline comments.Jul 9 2015, 9:24 PM
test/SemaCXX/cxx-concept-declaration.cpp
3

Please let me know if feel any tests are missing at this point.

nwilson updated this revision to Diff 29446.EditedJul 10 2015, 8:46 AM

As suggested by Aaron, modify phrasing of err_function_concept_not_defined
Remove redundant comment for concept declaration being definition

rsmith added inline comments.Jul 15 2015, 1:34 PM
include/clang/Basic/DiagnosticSemaKinds.td
1965–1966

Does this not also apply to variable concepts? For instance

template<typename T> extern concept bool b;
nwilson added inline comments.Jul 15 2015, 1:52 PM
include/clang/Basic/DiagnosticSemaKinds.td
1965–1966

Could we create a separate diagnostic for that? i.e.:

def err_variable_concept_not_intialized : Error<

"variable concept declaration requires initialization">;
rsmith added inline comments.Jul 17 2015, 5:03 PM
include/clang/Basic/DiagnosticSemaKinds.td
1965–1966

That seems fine to me.

hubert.reinterpretcast edited edge metadata.

It seems all of the raised issues have been resolved; LGTM.

This revision is now accepted and ready to land.Jul 20 2015, 3:30 PM
hubert.reinterpretcast retitled this revision from [CONCEPTS] Creating Diagnostics for ill-formed function concept declaration to [CONCEPTS] Add diagnostics: non-defining function; non-namespace scope.Jul 21 2015, 8:03 PM
hubert.reinterpretcast updated this object.
hubert.reinterpretcast edited edge metadata.