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

Thu Jun 15 03:48:44 PDT 2023

ChuanqiXu added a comment.

In D153003#4424004 <https://reviews.llvm.org/D153003#4424004>, @Hahnfeld wrote:

> In D153003#4423926 <https://reviews.llvm.org/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 could also normalize the `Kind`, for example from `QualifiedTemplate` to `Template` (which is the case I care about).

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.

>> 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 attached to named modules. (And this is also a defect that now we don't diagnose it correctly).
>
> I'm not sure I understand, could you elaborate? "correct" as in "the merge error is right" or "should work as written". Also there are a number of `export struct` in the tests, but you are saying this should not be included in the modules? How would this work?

I mean the error is correct. The test case itself is invalid. We shouldn't declare it as `expected-no-diagnostics`. The link above says the reason. The same declaration in multiple module purview is invalid.

>> Even if you put them into the global module fragment, I think we should try to look other places like `isSameEntity` to solve this.
>
> Sure, but this first requires the hash to be identical, no? My understanding is that two (structurally) identical Decls must always hash to the same value, but the same value does not imply that `isSameEntity` and friends eventually agree (due to hash collisions).

Yeah, it will be better to be identical. I think the major problem is that the patch misses information instead of adding more information.

> , but the same value does not imply that `isSameEntity` ...

No. IsSameEntity is a weaker check. It simply checks whether two named decls refers to the same `name`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153003