[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI
Shoaib Meenai via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 9 11:33:23 PST 2018
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.
More information about the cfe-commits