[PATCH] [CONCEPTS] Parsing of concept keyword

[nwilson]

Summary

This change adds parsing for the concept keyword in a declaration and tracks the location. Diagnostic testing added for invalid use of concept keyword.

[hubert.reinterpretcast]

Some issues with the test case; minor issues otherwise.

I would suggest testing the following as well:

template <typename T>
struct A { typedef bool Boolean; };

template <int N>
A<void>::Boolean concept C3(!0);

template <typename T, int = 0>
concept auto C4(void) -> bool { return true; }

constexpr int One = 1;

template <typename>
static concept decltype(!0) C5{ bool(One) };

lib/Parse/ParseDecl.cpp

4437

I'm not sure what C++TS1 is meant to refer to since the Concepts TS is not TS1.

test/Parser/cxx-concepts-value-function.cpp

1 ↗

(On Diff #27910)

I am not familiar with the 0|1 syntax being used here; my local copy of "lit" interprets it as a pipe to a command "1". Perhaps this should be two run lines?

4 ↗

(On Diff #27910)

The usual way to express this is "function concepts and variable concepts".

11 ↗

(On Diff #27910)

-DDIAG=0 would still trigger the #ifdef DIAG here (which explains why the test file might pass with expected-no-diagnostics).

[hubert.reinterpretcast]

test/Parser/cxx-concepts-value-function.cpp

11 ↗

(On Diff #27910)

I meant without expected-no-diagnostics somewhere.

[nwilson]

lib/Parse/ParseDecl.cpp

4437

I'll fix that. Any preference on a comment? C++14 Concepts-ts?

test/Parser/cxx-concepts-value-function.cpp

1 ↗

(On Diff #27910)

Yeah, I was trying to keep it on one line. I'll change it to two.

4 ↗

(On Diff #27910)

Thanks. I'll make the change for the correct verbiage.

11 ↗

(On Diff #27910)

Hmm, yeah, I missed that and thought it was working. Is there a better way to use/suppress the diagnostic cases?

[hubert.reinterpretcast]

lib/Parse/ParseDecl.cpp

4437

// C++ Concepts TS - concept

would work.

test/Parser/cxx-concepts-value-function.cpp

11 ↗

(On Diff #27910)

#if DIAG instead of #ifdef would work. I guess expected-no-diagnostics could be in the #else for that.

[rsmith]

lib/Sema/DeclSpec.cpp

899

Why just a warning? [dcl.spec]/2 seems to apply to concept just like any other decl-specifier.

test/Parser/cxx-concepts-value-function.cpp

3–4 ↗

(On Diff #27910)

We typically use BCPL //-comments in our C++ testcases, with no line-of-*s.

11 ↗

(On Diff #27910)

Just run the test once, and remove the #ifdef.

15 ↗

(On Diff #27910)

Which of the various errors on this line are you trying to test here?

[nwilson]

lib/Sema/DeclSpec.cpp

899

Wouldn't this be similar to constexpr (above)? Do you think it should error out?

test/Parser/cxx-concepts-value-function.cpp

3–4 ↗

(On Diff #27910)

Okay. I'll make the fix.

11 ↗

(On Diff #27910)

I'll submit a patch for this shortly so you guys can see if this explanation doesn't make sense. But, how about if I use the directives likes this:

#if DIAG == 0
RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s -DDIAG=0
expected-no-diagnostics

• test cases ***

#elif DIAG == 1
// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s -DDIAG=1

• test cases ***

#endif

15 ↗

(On Diff #27910)

All of those diagnostics are generated, so I guess all of them.

Is there a better way to do it? Should I only handle one?

[rsmith]

lib/Sema/DeclSpec.cpp

899

Yes, it should. (We don't yet implement DR1830 so the pre-existing cases aren't caught yet.)

test/Parser/cxx-concepts-value-function.cpp

11 ↗

(On Diff #27910)

What are you trying to achieve by running the test twice?

15 ↗

(On Diff #27910)

It'd be clearer to have a separate declaration for each different case you're trying to test, so that someone who causes the test to fail (say, by changing the parser's error recovery) can understand whether they've actually broken something.

[hubert.reinterpretcast]

test/Parser/cxx-concepts-value-function.cpp

11 ↗

(On Diff #27910)

The presence of errors in a file may prevent further processing. Such further processing is not guaranteed to apply only to code which occurs lexically after the location of the error. By avoiding diagnostic cases in one of the runs, we can be more confident that the cases which should not receive diagnostics would not do so (instead of merely having the unexpected diagnostics conveniently preempted).

[rsmith]

test/Parser/cxx-concepts-value-function.cpp

11 ↗

(On Diff #27910)

Failure to recover properly from an error would be a bug -- the parse of later unrelated code should not be affected by an earlier failure, except in extreme cases. The same argument would also imply that we should not have two "expected to fail" tests in a row in one test file, because the earlier failure could conceivably prevent the later failure from happening (or the later failure might only happen when the earlier failure does).

The valid cases will be tested more thoroughly in higher layers (for instance, testing that semantic analysis can use a concept to select an overload and that IR generation can generate correct IR for use of a concept), so I'm not at all worried about those later tests failing to catch a bug where a trivial concept definition leads to an unexpected error.

As a more general observation: there is an established pattern for these kinds of test (and many other things) in Clang. If our established pattern is wrong, we should deal with that by discussing the pattern and how to fix it, then fixing it globally, not by deviating from that pattern in an individual change (that will lead to a fragmented and harder-to-maintain codebase).

[hubert.reinterpretcast]

test/Parser/cxx-concepts-value-function.cpp

11 ↗

(On Diff #27910)

Okay; thanks for the explanation.

[nwilson]

This is an update for the comments in the review: make the // comments clearer/correct when referring to C++ concepts (values and functions), use warning diagnostic for duplicate declaration extension and add associated test cases, added more test cases, and removed the diagnostic directives.

[nwilson]

test/Parser/cxx-concepts-value-function.cpp

30 ↗

(On Diff #27993)

Is there a better way to list multiple diagnostics that would occur from the same declaration? When separating out each declaration for each test case such as:

template< concept T > concept bool D2 { return true; } expected-error {{unknown type name 'T'}}
template< concept T > concept bool D3 { return true; } expected-error {{expected expression}}
template< concept T > concept bool D4 { return true; } // expected-error {{expected ';' at end of declaration}}

clang complains:
'error' diagnostics seen but not expected:

and lists all of the diagnostics.

[hubert.reinterpretcast]

lib/Sema/DeclSpec.cpp

899

This is still just a warning without -pedantic-errors; I'm okay with it if Richard is.

[rsmith]

test/Parser/cxx-concepts-value-function.cpp

1 ↗

(On Diff #27993)

The name of this test file could be better. Something like 'cxx-concept-declaration.cpp', maybe, if you're intending on putting tests for the other parsing changes (requires-clauses, requires-expressions, concept-introducer syntax, ...) into separate files?

30 ↗

(On Diff #27993)

Try changing your test so each case only fails in one way. Like:

template< concept T > concept bool D2 = true; // expected-error {{unknown type name 'T'}}
template< typename T > concept bool D3 { return true; }; // expected-error {{expected expression}}
template< typename T > concept bool D4 { true } // expected-error {{expected ';' at end of declaration}}

... though the second and third cases are really testing variable/function disambiguation and don't have anything to do with concepts, so I'm not sure they belong here at all.

[rsmith]

lib/Sema/DeclSpec.cpp

899

This is consistent with how we treat other violations of the same rule; it seems fine to me.

[hubert.reinterpretcast]

test/Parser/cxx-concepts-value-function.cpp

1 ↗

(On Diff #27993)

There is a patch for requires-clauses here in case we want to coordinate the filenames: http://reviews.llvm.org/D10462.

[nwilson]

test/Parser/cxx-concepts-value-function.cpp

1 ↗

(On Diff #27993)

Yeah, I wasn't sure about the file name. I'd be okay with cxx-concept-declaration.cpp.

Hubert - do you have any preference?

30 ↗

(On Diff #27993)

Ahh, yeah, that makes sense. Thanks. I'll take the last two out in that case, and add more when adding actual diagnostics for ill formed concept declarations.

[hubert.reinterpretcast]

test/Parser/cxx-concepts-value-function.cpp

1 ↗

(On Diff #27993)

Sounds good.

[nwilson]

• Renaming test file name to cxx-concept-declaration.cpp
• Removing tests D2 and D3 as they're not applicable

[nwilson]

Ping

[rsmith]

LGTM

include/clang/Sema/DeclSpec.h

private:
   unsigned FS_noreturn_specified : 1;
 
   // friend-specifier
   unsigned Friend_specified : 1;
 
   // constexpr-specifier
   unsigned Constexpr_specified : 1;
 
+  // concept-specifier
+  unsigned Concept_specified : 1;
+
   union {
     UnionParsedType TypeRep;
     Decl *DeclRep;
     Expr *ExprRep;
   };
 
   // attributes.
   ParsedAttributes Attrs;
private:
   /// typename, then this is the location of the named type (if present);
   /// otherwise, it is the same as TSTLoc. Hence, the pair TSTLoc and
   /// TSTNameLoc provides source range info for tag types.
   SourceLocation TSTNameLoc;
   SourceRange TypeofParensRange;
   SourceLocation TQ_constLoc, TQ_restrictLoc, TQ_volatileLoc, TQ_atomicLoc;
   SourceLocation FS_inlineLoc, FS_virtualLoc, FS_explicitLoc, FS_noreturnLoc;
   SourceLocation FS_forceinlineLoc;
-  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc;
+  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc, ConceptLoc;
 
   WrittenBuiltinSpecs writtenBS;
   void SaveWrittenBuiltinSpecs();
 
   ObjCDeclSpec *ObjCQualifiers;
 
   static bool isTypeRep(TST T) {
     return (T == TST_typename || T == TST_typeofType ||
: StorageClassSpec(SCS_unspecified),
       TypeQualifiers(TQ_unspecified),
       FS_inline_specified(false),
       FS_forceinline_specified(false),
       FS_virtual_specified(false),
       FS_explicit_specified(false),
       FS_noreturn_specified(false),
       Friend_specified(false),
       Constexpr_specified(false),
+      Concept_specified(false),
       Attrs(attrFactory),
       ProtocolQualifiers(nullptr),
       NumProtocolQualifiers(0),
       ProtocolLocs(nullptr),
       writtenBS(),
       ObjCQualifiers(nullptr) {
   }
   ~DeclSpec() {
bool setFunctionSpecNoreturn(SourceLocation Loc, const char *&PrevSpec,
                                unsigned &DiagID);
 
   bool SetFriendSpec(SourceLocation Loc, const char *&PrevSpec,
                      unsigned &DiagID);
   bool setModulePrivateSpec(SourceLocation Loc, const char *&PrevSpec,
                             unsigned &DiagID);
   bool SetConstexprSpec(SourceLocation Loc, const char *&PrevSpec,
                         unsigned &DiagID);
+  bool SetConceptSpec(SourceLocation Loc, const char *&PrevSpec,
+                      unsigned &DiagID);
 
   bool isFriendSpecified() const { return Friend_specified; }
   SourceLocation getFriendSpecLoc() const { return FriendLoc; }
 
   bool isModulePrivateSpecified() const { return ModulePrivateLoc.isValid(); }
   SourceLocation getModulePrivateSpecLoc() const { return ModulePrivateLoc; }
   
   bool isConstexprSpecified() const { return Constexpr_specified; }
   SourceLocation getConstexprSpecLoc() const { return ConstexprLoc; }
 
+  bool isConceptSpecified() const { return Concept_specified; }
+  SourceLocation getConceptSpecLoc() const { return ConceptLoc; }
+
   void ClearConstexprSpec() {
     Constexpr_specified = false;
     ConstexprLoc = SourceLocation();
   }
 
+  void ClearConceptSpec() {
+    Concept_specified = false;
+    ConceptLoc = SourceLocation();
+  }
+
   AttributePool &getAttributePool() const {
     return Attrs.getPool();
   }
 
   /// \brief Concatenates two attribute lists.
   ///
   /// The GCC attribute syntax allows for the following:
   ///

lib/Parse/ParseDecl.cpp

case tok::kw___module_private__:
       isInvalid = DS.setModulePrivateSpec(Loc, PrevSpec, DiagID);
       break;
 
     // constexpr
     case tok::kw_constexpr:
       isInvalid = DS.SetConstexprSpec(Loc, PrevSpec, DiagID);
       break;
 
+    // concept
+    case tok::kw_concept:
+      isInvalid = DS.SetConceptSpec(Loc, PrevSpec, DiagID);
+      break;
+
     // type-specifier
     case tok::kw_short:
       isInvalid = DS.SetTypeSpecWidth(DeclSpec::TSW_short, Loc, PrevSpec,
                                       DiagID, Policy);
       break;
     case tok::kw_long:
       if (DS.getTypeSpecWidth() != DeclSpec::TSW_long)
         isInvalid = DS.SetTypeSpecWidth(DeclSpec::TSW_long, Loc, PrevSpec,
case tok::kw_typeof:
 
     // GNU attributes.
   case tok::kw___attribute:
 
     // C++11 decltype and constexpr.
   case tok::annot_decltype:
   case tok::kw_constexpr:
 
+    // C++ Concepts TS - concept
+  case tok::kw_concept:
+
     // C11 _Atomic
   case tok::kw__Atomic:
     return true;
 
     // GNU ObjC bizarre protocol extension: <proto1,proto2> with implicit 'id'.
   case tok::less:
     return getLangOpts().ObjC1;
 

lib/Parse/ParseTentative.cpp

case tok::kw_decltype:
 
     // decl-specifier:
     //   storage-class-specifier
     //   type-specifier
     //   function-specifier
     //   'friend'
     //   'typedef'
     //   'constexpr'
+    //   'concept'
   case tok::kw_friend:
   case tok::kw_typedef:
   case tok::kw_constexpr:
+  case tok::kw_concept:
     // storage-class-specifier
   case tok::kw_register:
   case tok::kw_static:
   case tok::kw_extern:
   case tok::kw_mutable:
   case tok::kw_auto:
   case tok::kw___thread:
   case tok::kw_thread_local:

lib/Sema/DeclSpec.cpp

if (Constexpr_specified) {
     PrevSpec = "constexpr";
     return true;
   }
   Constexpr_specified = true;
   ConstexprLoc = Loc;
   return false;
 }
 
+bool DeclSpec::SetConceptSpec(SourceLocation Loc, const char *&PrevSpec,
+                              unsigned &DiagID) {
+  if (Concept_specified) {
+    DiagID = diag::ext_duplicate_declspec;
+    PrevSpec = "concept";
+    return true;
+  }
+  Concept_specified = true;
+  ConceptLoc = Loc;
+  return false;
+}
+
 void DeclSpec::setProtocolQualifiers(Decl * const *Protos,
                                      unsigned NP,
                                      SourceLocation *ProtoLocs,
                                      SourceLocation LAngleLoc) {
   if (NP == 0) return;
   Decl **ProtoQuals = new Decl*[NP];
   memcpy(ProtoQuals, Protos, sizeof(Decl*)*NP);
   ProtocolQualifiers = ProtoQuals;

test/Parser/cxx-concept-declaration.cpp

+
+// Support parsing of function concepts and variable concepts
+
+// RUN:  %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
+
+template<typename T> concept bool C1 = true;
+
+template<typename T> concept bool C2() { return true; }
+
+template<typename T>
+struct A { typedef bool Boolean; };
+
+template<int N>
+A<void>::Boolean concept C3(!0);
+
+template<typename T, int = 0>
+concept auto C4(void) -> bool { return true; }
+
+constexpr int One = 1;
+
+template <typename>
+static concept decltype(!0) C5 { bool(One) };
+
+template<typename T> concept concept bool C6 = true; // expected-warning {{duplicate 'concept' declaration specifier}}
+
+template<typename T> concept concept bool C7() { return true; } // expected-warning {{duplicate 'concept' declaration specifier}}
+
+concept D1 = true; // expected-error {{C++ requires a type specifier for all declarations}}
+
+template<concept T> concept bool D2 = true; // expected-error {{unknown type name 'T'}}