[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Nathan Sidwell via Phabricator via cfe-commits]

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:
> > 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'


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118034/new/

https://reviews.llvm.org/D118034