ChuanqiXu added a comment.
In D126907#3615807 <https://reviews.llvm.org/D126907#3615807>, @erichkeane
wrote:
> All tests pass now, I was able to get the template-template checks working
> correctly, and it passes all the tests I have available. @ChuanqiXu if you
> could review/run what tests you have, it would be greatly appreciated.
I've tested some of our workloads and libunifex (I know it contains a lot of
uses for concept). And all the tests passed now. So it looks like it wouldn't
cause regression failure.
The implementation **basically** looks good to me. (This is really large and I
can't be sure I don't miss something). Only some minor issues remained.
================
Comment at: clang/include/clang/AST/Decl.h:1944-1948
+ /// For non-templates this value will be NULL, unless this non-template
+ /// function declaration was declared directly inside of a function template,
+ /// in which case this will have a pointer to a FunctionDecl, stored in the
+ /// NamedDecl. For function declarations that describe a function template,
+ /// this will be a pointer to a FunctionTemplateDecl, stored in the
NamedDecl.
----------------
Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? The
NamedDecl* covers a lot of things. It looks more consistent.
================
Comment at: clang/include/clang/Sema/Sema.h:7056-7067
+ /*
+ // Keep track of whether we are evaluating a constraint.
+ unsigned ConstraintEvaluationDepth = 0;
+
+ class ConstraintEvalRAII {
+ Sema &S;
+
----------------
Remove this before landing.
================
Comment at: clang/include/clang/Sema/Sema.h:7070
public:
+ // bool IsEvaluatingAConstraint() { return ConstraintEvaluationDepth > 0; }
const NormalizedConstraint *
----------------
ditto
================
Comment at: clang/lib/AST/Decl.cpp:3788
"Member function is already a specialization");
- TemplateOrSpecialization = Template;
+ TemplateOrSpecialization = static_cast<NamedDecl *>(Template);
+}
----------------
`static_cast` is rarely used in clang/LLVM to me. I know the reason is that
`TemplateOrSpecialization` contains a `NamedDecl*` member. I guess it might be
better to refactor it.
================
Comment at: clang/lib/AST/DeclBase.cpp:286-304
const DeclContext *Decl::getParentFunctionOrMethod() const {
for (const DeclContext *DC = getDeclContext();
DC && !DC->isTranslationUnit() && !DC->isNamespace();
DC = DC->getParent())
if (DC->isFunctionOrMethod())
return DC;
----------------
How about:
================
Comment at: clang/lib/Sema/SemaConcept.cpp:63
+
+ ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS) {
+ return recreateBinOp(SemaRef, LHS, const_cast<Expr *>(getRHS()));
----------------
================
Comment at: clang/lib/Sema/SemaConcept.cpp:67
+
+ ExprResult recreateBinOp(Sema &SemaRef, ExprResult LHS, ExprResult RHS) {
+ assert((isAnd() || isOr()) && "Not the right kind of op?");
----------------
================
Comment at: clang/lib/Sema/SemaConcept.cpp:186
+ return BO.recreateBinOp(S, LHSRes, RHSRes);
} else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr)) {
+ // These aren't evaluated, so we don't care about cleanups, so we can just
----------------
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2339
if (const auto *TC = TTP->getTypeConstraint())
- SemaRef.SubstTypeConstraint(NewTTP, TC, Args);
+ SemaRef.SubstTypeConstraint(NewTTP, TC, Args, false);
if (TTP->hasDefaultArgument()) {
----------------
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126907/new/
https://reviews.llvm.org/D126907
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
