[PATCH] D40381: Parse concept definition

[Faisal Vali via Phabricator via cfe-commits]

faisalv requested changes to this revision.
faisalv added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Sema/SemaTemplate.cpp:3899
+  // constraint expressions right now.
+  return Template->getConstraintExpr();
+}
----------------
saar.raz wrote:
> faisalv wrote:
> > I don't think we want to just return the constraint expr? I think we would need to create another conceptspecializationDecl node and nest it within a DeclRefExpr?  But once again, I think we should defer this to another patch - no?
> This was meant as a placeholder. The next patch (D41217) replaces this function, I don't think it is that much of a big deal what happens here in this patch.
This: " I don't think it is that much of a big deal what happens here in this patch." I think is poor practice in an open source project when you're not sure when the next patch will land.  

I think, when features are implemented incrementally - each patch should diagnose the yet to be implemented features - and error out as gracefully as possible.  So for instance replacing the body of this function with an appropriate diagnostic (that is then removed in future patches) would be the better thing to do here.



================
Comment at: lib/Sema/SemaTemplate.cpp:7713
+
+  if (!ConstraintExpr->isTypeDependent() &&
+      ConstraintExpr->getType() != Context.BoolTy) {
----------------
saar.raz wrote:
> faisalv wrote:
> > Consider refactoring these checks on constraint expressions into a separate function checkConstraintExpression (that we can also call from other contexts such as requires-clauses and nested requires expressions)?
> I did that in the upcoming patches, no need to do it here as well.
Once again - relying on a future patch (especially without a clear FIXME) to tweak the architectural/factorization issues that you have been made aware of (and especially those, such as this, that do not require too much effort), is not practice I would generally encourage.  

So changyu, if you agree that these suggestions would improve the quality of the patch and leave clang in a more robust state (maintenance-wise or execution-wise), then please make the changes.  If not, please share your thoughts as to why these suggestions would either not be an improvement or be inconsistent with the theme of this patch.

Thanks!


https://reviews.llvm.org/D40381