smeenai added a comment. In https://reviews.llvm.org/D52674#1293180, @rjmccall wrote:
> In https://reviews.llvm.org/D52674#1291284, @smeenai wrote: > > > In https://reviews.llvm.org/D52674#1281747, @rjmccall wrote: > > > > > In https://reviews.llvm.org/D52674#1271814, @smeenai wrote: > > > > > > > @rjmccall I prototyped the ForRTTI parameter approach in > > > > https://reviews.llvm.org/D53546. It could definitely be cleaned up a > > > > bit, but it demonstrates the problems I saw with the added parameter. > > > > Namely, `mangleType(QualType, SourceRange, QualifierMangleMode)` has a > > > > bunch of additional logic which is needed for correctness, so we need > > > > to thread the parameter through the entire chain, and the only way I > > > > could think of doing that without adding the parameter to every single > > > > `mangleType` overload was to have an additional switch to dispatch to > > > > the overloads with the added ForRTTI parameter, which is pretty ugly. > > > > > > > > As I see it, there are a few ways to proceed here: > > > > > > > > - The approach in https://reviews.llvm.org/D53546 (cleaned up a bit). I > > > > think it's pretty ugly, but you may have suggestions on how to do it > > > > better. > > > > - In the `mangleType` overload for `ObjCObjectPointerType`, skipping > > > > the generic `mangleType` dispatch and going directly to either the > > > > `ObjCObjectType` or `ObjCInterfaceType` overloads. That avoids the > > > > ugliness in the generic `mangleType`, but it also requires us to > > > > duplicate some logic from it in the `ObjCObjectPointerType` overload, > > > > which doesn't seem great either. > > > > - Maintaining `ForRTTI` state in the mangler instead of threading a > > > > parameter through (I'm generally not a fan of state like that, but it > > > > might be cleaner than the threading in this case?) > > > > - Just sticking with what I'm doing in this patch. > > > > > > > > What do you think? > > > > > > > > > Sorry for the delay. I think duplicating the dispatch logic for > > > `ObjCObjectPointerType` is probably the best approach; the pointee type > > > will always an `ObjCObjectType` of some sort, and there are only two > > > kinds of those. > > > > > > Sorry, I'm still not sure how this will work. > > > > Duplicating the dispatch logic for `ObjCObjectPointerType` ends up looking > > like https://reviews.llvm.org/P8114, which is fine. However, when we're > > actually mangling RTTI or RTTI names, we're still going through the main > > `mangleType(QualType, SourceRange, QualifierMangleMode)` overload, which > > still requires us to thread `ForRTTI` through that function, which still > > requires us to duplicate the switch in that function (to handle the > > `ForRTTI` case, since the other switch is generated via TypeNodes.def and > > not easily modifiable), which is the main ugliness in my opinion. Do you > > also want me to add special dispatching to `mangleCXXRTTI` and > > `mangleCXXRTTIName` to just call the `ObjCObjectPointerType` function > > directly when they're dealing with that type? That's certainly doable, but > > at that point just keeping some state around in the demangler starts to > > feel cleaner, at least to me. > > > Well, you could check for an `ObjCObjectPointerType` in the top-level routine. > > But yeah, probably the cleanest thing to do would to be push the state > through the main dispatch. Don't push a single `bool` through, though; make > a `TypeManglingOptions` struct to encapsulate it, in case we have a similar > problem elsewhere. It should have a method for getting options that should > be propagated down to component type manglings, and that method can drop the > "for RTTI" bit; that's the part that `ObjCObjectPointerType` can handle > differently. All right, the struct is a good idea. It would require adding the `TypeManglingOptions` parameter to all the `mangleType` overloads (unless we want to stick with the second switch in the main `mangleType` dispatch function, but that seems super ugly, especially if we're doing a more general solution), so I wanted to confirm @rnk is okay with adding a parameter to all those overloads as well before proceeding. Repository: rC Clang https://reviews.llvm.org/D52674 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
