[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. In D153003#4463426 , @v.g.vassilev wrote: > In D153003#4462388 , @ChuanqiXu > wrote: > >>> Oh, I guess we're somehow adding a semi-resolved form of the base class >>> type of D into t

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

v.g.vassilev added a comment. In D153003#4462388 , @ChuanqiXu wrote: >> Oh, I guess we're somehow adding a semi-resolved form of the base class type >> of D into the ODR hash, which then includes details of the >> using-declaration. That seems wrong --

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. > Oh, I guess we're somehow adding a semi-resolved form of the base class type > of D into the ODR hash, which then includes details of the using-declaration. > That seems wrong -- we should either (preferably) be including only the > syntactic form of the base specif

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

rsmith added a comment. In D153003#4458323 , @ChuanqiXu wrote: > In D153003#4456595 , @rsmith wrote: > >> How to we even get into the ODR hasher here? I thought we only applied it to >> function and class definit

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. In D153003#4458366 , @Hahnfeld wrote: > In D153003#4458323 , @ChuanqiXu > wrote: > >> In D153003#4456595 , @rsmith wrote: >> >>> I think the be

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld added a comment. In D153003#4458323 , @ChuanqiXu wrote: > In D153003#4456595 , @rsmith wrote: > >> I think the behavior change for the testcase here is correct, though I'm not >> sure that the patch is g

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. In D153003#4456595 , @rsmith wrote: > I think the behavior change for the testcase here is correct, though I'm not > sure that the patch is getting that behaviour change in the right way. Per > [temp.type]/1.4 (http://eel.is/c

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

rsmith added a comment. I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4), > Two template-ids are the same if [...] their corresponding te

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. Yeah, but I feel the most important problem for the patch is that the reproducer is not valid according to the wording. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153003/new/ https://reviews.llvm.org/D153003

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld abandoned this revision. Hahnfeld added a comment. Ok, I understand that fixing `ODRHash` is the wrong approach for our needs - we'll likely need to implement a custom hashing of template arguments to work as a lookup for lazy loading. Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. In D153003#4427731 , @Hahnfeld wrote: >> We can't say this abstractly. It should be fine to work in ODRHash for C++20 >> modules issues as long as we don't lose informations. > > I honestly don't understand this: For the approa

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld added a comment. > We can't say this abstractly. It should be fine to work in ODRHash for C++20 > modules issues as long as we don't lose informations. I honestly don't understand this: For the approach to work, we would need to hash the two cases of an unqualified and a qualified temp

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. > I was thinking to "solve" this by making them hash to the same value, but I'm > hearing that using ODRHash for this purpose is the wrong approach to begin > with? We can't say this abstractly. It should be fine to work in ODRHash for C++20 modules issues as long as

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld added a comment. In D153003#4427412 , @ChuanqiXu wrote: > An important node here is that ODRHash is used to check the AST Nodes are > keeping the same across compilations. There is gap to use ODRHash to check > the semantical equality. Oh ok..

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. > what I'm asking is why we need to differentiate between the two, ie in which > cases do we want these two to have a different hash? I don't understand. It should a basic requirement for different AST entities to have different values. > The way I understand your st

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld added a comment. > For example, the template name with QualifiedTemplate kind has different hash > name with the name with DependentTemplate kind. But it is not true after the > patch. Yes, I do understand. However, what I'm asking is why we need to differentiate between the two, ie i

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. In D153003#4424004 , @Hahnfeld wrote: > In D153003#4423926 , @ChuanqiXu > wrote: > >> This looks not so good. In this way, we can't disambiguate other template >> types. At least we ad

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld added a comment. In D153003#4423926 , @ChuanqiXu wrote: > This looks not so good. In this way, we can't disambiguate other template > types. At least we added the kind and a TODO here. Which template name types would we need to disambiguate? We

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

ChuanqiXu added a comment. This looks not so good. In this way, we can't disambiguate other template types. At least we added the kind and a TODO here. BTW, the attached test case is in fact in correct. See https://eel.is/c++draft/basic.def.odr#14.3, such mergeable declarations shouldn't be at

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

Hahnfeld created this revision. Hahnfeld added reviewers: rtrieu, vsapsai, ChuanqiXu, Bigcheese. Herald added a project: All. Hahnfeld requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. For hashing, we should not differentiate between template