[PATCH] D11027: [CONCEPTS] Creating Diagnostics for ill-formed function concept declaration

[Nathan Wilson]

nwilson added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1964
@@ +1963,3 @@
+def err_invalid_concept_scope : Error<
+  "concept declaration required to be in namespace scope">;
+def err_function_concept_not_defined : Error<
----------------
hubert.reinterpretcast wrote:
> 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.
Sure, change it to something similar.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1966
@@ +1965,3 @@
+def err_function_concept_not_defined : Error<
+  "function concept requires definition">;
+
----------------
hubert.reinterpretcast wrote:
> 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.
Makes sense. I'll change the wording.

================
Comment at: lib/Sema/SemaDecl.cpp:7447
@@ +7446,3 @@
+      // template, declared in namespace scope
+      if (!NewFD->getParent()->getRedeclContext()->isFileContext()) {
+        Diag(D.getIdentifierLoc(), diag::err_invalid_concept_scope);
----------------
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(...)
}

================
Comment at: lib/Sema/SemaDecl.cpp:7453
@@ +7452,3 @@
+      // C++ Concepts TS [dcl.spec.concept]p1: A concept definition refers to a
+      // function concept and its definition
+      if (!D.isFunctionDefinition()) {
----------------
hubert.reinterpretcast wrote:
> 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".
That makes sense. I'll make the change.

================
Comment at: test/SemaCXX/cxx-concept-declaration.cpp:5
@@ +4,3 @@
+
+template<typename T> concept decltype(!0) C2 = true;
+
----------------
hubert.reinterpretcast wrote:
> Same comment as on line 3.
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.


http://reviews.llvm.org/D11027