[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

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.


To be clear, we would need to duplicate (or factor out into a common function) some mangling logic as well, because e.g. adding the `E`

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.


Repository:
  rC Clang

https://reviews.llvm.org/D52674