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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/DeclTemplate.h:373-391
+class TemplateDeclWithACBase {
+protected:
+  TemplateDeclWithACBase() = default;
+
+  ConstrainedTemplateDeclInfo CTDInfo;
+};
+
----------------
This mechanism seems unnecessary to me; allocating the `ConstrainedTemplateDeclInfo` separately seems a lot simpler. Forcing this and the template into a single allocation is unlikely to help anything since we use a slab allocator (which is going to lay the objects out the same way this template trick does, unless we hit the end of a slab).


================
Comment at: lib/Sema/SemaTemplate.cpp:1178
+      if (!(CurAC || PrevAC))
+        return false; // nothing to check
+      if (CurAC && PrevAC) {
----------------
[nit] Comments should be full sentences: capitalized and ending in a period.


================
Comment at: lib/Sema/SemaTemplate.cpp:1299-1300
 
+  // Attach the associated constraints when the declaration will not be part of
+  // a decl chain
+  Expr *const ACtoAttach =
----------------
Is there a reason you don't want to store the associated constraints that were specified on a redeclaration? I'd expect that to hurt tools that want source fidelity (for instance, a renaming tool will want to be able to find all the references to a particular name, even in a //requires-clause// on a redeclaration of a template).


https://reviews.llvm.org/D25674