[PATCH] Add MicrosoftVFTableContext to AST

[John McCall]

On Jul 30, 2013, at 2:48 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> 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.

How so?  Itanium’s v-tables are still logically broken up into separate
components, such that an access through any one v-table pointer only
ever accesses a particular component.  You could very easily change
Itanium to allocate all the v-tables in separate symbols without any need
to change the code for virtual calls.  It’s just that Itanium is trying to
optimize the number of separate symbols required.

>> +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!

Sure, but — oh, I see, we just need to return the offset from the complete
object here, and if we need a thunk to adjust to that, then the calling logic
will handle 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.

Yeah, C++03 has some pointless restrictions about template arguments.

Also, we’ve found that MSVC tends to miscompile local classes, so pulling
things out is for the best.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130730/fd610864/attachment.html>