[PATCH] Add MicrosoftVFTableContext to AST

[Timur Iskhodzhanov]

2013/7/30 John McCall <rjmccall at apple.com>:
> On Jul 29, 2013, at 3:40 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>
>  Update the return adjustment tests to make sure we put the covariant-return
> override after the methods defined in the base which defines the overridden
> function.
>
>  Also, add back the implicit test expectations so it's a more obvius example
> of adding a slot to the vftable of a non-primary base subobject.
>
>
> +/// Visit all the methods overridden by the given method recursively,
> +/// in a depth-first pre-order. The Visitor's visitor method returns a bool
> +/// indicating whether to continue the recursion for the given overridden
> +/// method.
>
> It’s worth spelling out that “false” stops the iteration.

Done

> +namespace {
> +  struct OverriddenMethodsCollector {
> +    OverriddenMethodsSetTy *Methods;
> +
> +    bool visit(const CXXMethodDecl *MD) {
> +      return Methods->insert(MD);;
>
> Double semi-colons, and please add a comment like:
>   // Don’t recurse on this method if we’ve already collected it.
>
> +//  2. Each subobject with a vfptr gets its own vftable rather than an
> address
> +//     point in a single vtable shared between all the subobjects.
>
> I’d make it clear that this is about vftable *symbols*

Done

> and doesn’t itself affect, say, the v-call mechanism.

Hm, not sure what you've meant here - they somewhat do affect the
vcall mechanism.
Feel free to fix my updated wording after commit or let's postpone
this till the review of the CodeGen changes.

> +//     value.  Therefore, if the offset isn't really constant (i.e. if a
> virtual
>
> This is just an example of how this can happen, so you should use
> “e.g." instead of “i.e.”.

Good point, done.

> +//  5. Instead of VTT and constructor vtables, vtordisps are emitted into
> the
> +//     class layout if a class has
> +//      a) a user-defined ctor/dtor
> +//     and
> +//      b) a method overriding a method in a virtual base.
>
> The main thing that replaces VTT and construction v-tables is having
> a separate vb-table.

OK, added that.

> +CharUnits
> +VFTableBuilder::ComputeThisOffset(const CXXMethodDecl *MD,
> +                                  BaseSubobject Base,
> +                                  FinalOverriders::OverriderInfo Overrider)
> {
> +  if (isa<CXXDestructorDecl>(MD))
> +    return CharUnits();
> +
>
> Wait, why don’t destructors require this-offsets?

Nope, as the virtual dtor's final overrider is emitted for the MDC,
which has 0 offset.
I've added a comment for that!

> Otherwise, this is looking good.

Thanks!
Committed as r187409.

I also had to move one of the visitors out of the
VFTableBuilder::ComputeThisOffset body to avoid compiler warning.
The change is pretty trivial so I'm pretty sure it's fine to commit
without a re-review.
The only issue is the visitor's name might be weird - I couldn't come
up with a good one, sorry.
Feel free to rename if you have a better version.