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

Thu Jun 15 02:42:48 PDT 2023

Hahnfeld added a comment.

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

> 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?

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153003