[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:
> 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.


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

https://reviews.llvm.org/D118034