[PATCH] D25674: [Concepts] Class template associated constraints

[Richard Smith via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/DeclTemplate.h:412-417
+  /// \brief The template parameter list and optional requires-clause
+  /// associated with this declaration.
+  ///
+  /// The boolean value indicates whether this particular declaration has an
+  /// attached \c Expr representing the associated constraints of the template.
+  llvm::PointerIntPair<TemplateParameterList *, 1, bool> TemplateParams;
----------------
Rather than tail-allocating the associated constraints (which will be an inconvenience for every class that derives from `TemplateDecl`), can you store this as a `PointerUnion` of a `TemplateParameterList*` and a pointer to a struct wrapping a `TemplateParameterList*` and the associated constraints expression? I'm not really concerned about paying one extra pointer per associated constraints expression we store (and I suspect we may want to store this as something richer than an expression at some point).


================
Comment at: lib/Sema/SemaTemplate.cpp:1158
+      if (!(CurAC || PrevAC))
+        continue; // nothing to check
+      if (CurAC && PrevAC) {
----------------
I'm not a big fan of `continue` as a proxy for a `goto`. It looks like you could rearrange the logic here to avoid this:

  bool ConstraintMismatch = false;
  if ((bool)CurAC != (bool)PrevAC)
    ConstraintMismatch = true;
  else if (CurAC) {
    // profile and set ConstraintMismatch
  }
  if (ConstraintMismatch) Diag(...)

... or factor out a lambda to do the mismatch checking.


================
Comment at: test/CXX/concepts-ts/temp/temp.constr/temp.constr.decl/class-template-decl.cpp:1
+// RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s
+
----------------
I like the idea of separate subdirectories of test/CXX for TSes. Thanks :)


https://reviews.llvm.org/D25674