urnathan added inline comments.
================ Comment at: clang/lib/Sema/SemaTemplate.cpp:2658-2682 + Expr::EvalResult EVRX, EVRY; + if (!DefaultArgumentX->EvaluateAsConstantExpr(EVRX, C) || + !DefaultArgumentY->EvaluateAsConstantExpr(EVRY, C)) + return false; + + APValue VX = EVRX.Val, VY = EVRY.Val; + if (VX.getKind() != VY.getKind()) ---------------- ChuanqiXu wrote: > urnathan wrote: > > urnathan wrote: > > > ChuanqiXu wrote: > > > > urnathan wrote: > > > > > ChuanqiXu wrote: > > > > > > urnathan wrote: > > > > > > > I'm kind of surprised how complex this check is. Isn't there an > > > > > > > AST comparator available somewhere? > > > > > > I found ODRHash. I think it is much better now. > > > > > hm that suggests there there must be a comparator too -- this isn't a > > > > > cryptographically strong hash is it? How would the compiler > > > > > currently make use of 'definitely different' and 'probably the same' > > > > > without such a comparator? > > > > Yeah, I am sure there is not an such comparator. Or it has some methods > > > > like: `ASTContext::hasSameType` for type and > > > > `ASTContext::isSameEntity()` for Decl. But it lacks such methods now > > > > for Stmt and Expr. > > > > > > > > > How would the compiler currently make use of 'definitely different' > > > > > and 'probably the same' without such a comparator? > > > > > > > > Now it uses the two methods I listed above and ODRHash to compare. I > > > > think the two methods works for 'definitely different' and ODRHash > > > > works for 'probably the same'. So it's the reason why my previous > > > > implementation looks lengthy. Since I want to handle it by hand. (The > > > > previous method only works for simple Expr. I think it would be large > > > > work to implement comparator for whole Expr or Stmt). > > > Hm, how do template instantations work -- there must be some way of > > > determining 'is this instantation just here the same as one I've already > > > seen' > > also, the same functionality (with more generality) is needed for comparing > > regular function default arguments with multiple definitions in the GM. > > How is that done (or is it yet to be implemented?) > > also, the same functionality (with more generality) is needed for comparing > > regular function default arguments with multiple definitions in the GM. How > > is that done (or is it yet to be implemented?) > > We implemented ODR check in ASTReader by ODRHash. > > > Hm, how do template instantations work -- there must be some way of > > determining 'is this instantation just here the same as one I've already > > seen' > > First, previously we don't allow the redefinition of template default > argument. And for the case without template default argument, previously we > would try find the definition for the synthesized type, if we could find one, > use it. And if we couldn't find one, initialize one. > > --- > > Given we already checks ODR by ODRHash in ASTReader, I think what we do here > might not be bad. I agree that it is odd at the first sight to use Hash to > detect difference from the perspective of CS. But if this is what we used to > do, I think it is acceptable. indeed. I would be more comfortable with a comment by someone more familiar with this @rsmith ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118034/new/ https://reviews.llvm.org/D118034 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
