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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu 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())
----------------
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).


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

https://reviews.llvm.org/D118034