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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
