rsmith added a comment. In D106994#2911903 <https://reviews.llvm.org/D106994#2911903>, @vsapsai wrote:
> In D106994#2911508 <https://reviews.llvm.org/D106994#2911508>, @rsmith wrote: > >> For what it's worth, I think the right way to handle this in C is to >> properly implement the "compatible types" rule instead of trying to invent >> something ODR-ish: in C, struct definitions in different translation units >> are different types, but if they're structurally equivalent then they're >> compatible and you can implicitly pass an object of some type to a function >> accepting a compatible type. This would mean that we could have multiple, >> different types with the same name, and we'd need name lookup to deduplicate >> compatible types, but we wouldn't need to do any cross-module ODR-like >> struct merging. > > I agree that implementing the "compatible types" looks better as it models > the language more faithfully. And in the long run we might need to do that > anyway. Is there any work done for "compatible types" already? Or I can start > by creating a new type for a new definition with the same name and see how it > breaks the lookup? > > From pragmatic perspective we are pretty invested into this ODR-ish approach > and it's not clear how much work switching to "compatible types" would take. > So I'd like to continue with the definition merging and evaluate the effort > for "compatible types". That's why I'm curious what work is done already. I don't think there's been any real work done on a cross-TU implementation of compatible types. I also don't want that idea to get in the way of this patch, which seems like a clear improvement following our current approach. >> But assuming we want to keep the current ODR-in-C approach, this looks OK. >> There might be some places that assume the lexical and semantic >> `DeclContext` for a C `FieldDecl` are the same (etc) but I don't think >> there's a good way to find such things other than by testing this patch >> broadly. > > Are there any known signs for mixing lexical and semantic `DeclContext`? I > plan to test the change on our internal codebase, hopefully it'll help to > catch any remaining issues. The kinds of things I saw go wrong when we were bringing this up on the C++ side were generally in code that would walk the list of (say) fields of a record building up some information, and then attempt to look up a given `FieldDecl*` in that data structure. That can fail if fields get merged, because the lookup key may be a different redeclaration of the same field than the one found by walking the class's members. The fix is usually to add `getCanonicalDecl` calls in the right places. The sign of this kind of bug happening was usually a crash or assert, usually pretty close to where the problem was. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106994/new/ https://reviews.llvm.org/D106994 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
