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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
