Page MenuHomePhabricator

PR13236 - Microsoft compatibility: support __super specifier to access members of base classes
ClosedPublic

Authored by nikola on Jul 10 2014, 9:57 PM.

Details

Summary

This is a re-based version of Martin's patch from the bug report with few minor changes by me.

I don't think that we have to implement this feature identically to msvc, but I'm not opposed to it. I'd like to implement it in such a way that allows us to compile MFC code that depends on it. With that said here are some open questions.

  • MSDN says that __super can't be used with using declarations but current patch doesn't emit a diagnostic in this case. I'd say that we don't have to because since msvc doesn't allow this there's no code written that does it.
  • MSDN says that all accessible base class methods are considered during overload resolution with best one being selected. This is different from normal C++ rules where ordinary name lookup results in ambiguity. Current patch doesn't implement this logic (second test case fails) but similarly to my previous observation we might not need it as MFC doesn't use multiple inheritance. Client's might run into this issue in their classes, but in that case they can fix the code and move away from this extension.

What are your thoughts on this?

Diff Detail

Event Timeline

nikola updated this revision to Diff 11305.Jul 10 2014, 9:57 PM
nikola retitled this revision from to PR13236 - Microsoft compatibility: support __super specifier to access members of base classes.
nikola updated this object.
nikola edited the test plan for this revision. (Show Details)
nikola added reviewers: aaron.ballman, rnk, rsmith.
nikola added a subscriber: Unknown Object (MLST).
rnk added inline comments.Jul 11 2014, 2:47 PM
lib/AST/ItaniumMangle.cpp
804–806

This seems bad. IMO we can mangle this as a source name of super. It's in the implementer's namespace, so we should be OK. Please add a test for this. I think if you instantiate a template with a non-type template parameter using super you'll get this mangling.

lib/AST/NestedNameSpecifier.cpp
190–191

That seems wrong. It's dependent if there are dependent bases. Consider:

template <typename T>
struct A : T {
  __super::TypeFromBase x;
};

Maybe a __super specifier should store the RecordType that it will search inside of so that we can ask it this kind of question?

lib/Parse/ParseExprCXX.cpp
230

Please fix.

lib/Sema/SemaCXXScopeSpec.cpp
153

It might make more sense to treat this more like a TypeSpec NNS, and to return a stored tag decl here.

lib/Sema/SemaExpr.cpp
1953–1955 ↗(On Diff #11305)

I'm not sure what should happen here...

lib/Sema/TreeTransform.h
3067–3068

If we go the route of storing the tag decl of the class that __super was used in, we will end up wanting to transform that decl from a pattern to a specialization.

test/SemaCXX/MicrosoftExtensions.cpp
444

Missing semicolon and closing brace for namespace?

This could also use a lot more tests.

Thanks for the input guys, I have two questions:

  1. MSDN states "__super can only appear within the body of a member function." and yet msvc accepts it in a number of places (data member declaration, typedef, etc.). Do we wan't to be bug for bug compatible with msvc as I see that as major pain?
  1. I agree with Aaron that we should support the 'name found in multiple bases' scenario. In theory this is simple enough, but I don't know how hard it is to implement in practice. I guess this is a question for Richard, can I get ambiguous declarations from LookupResult (I see that this is possible) and do an overload resolution on them (this looks more difficult as I don't know how to access the call arguments)?
nikola updated this revision to Diff 12386.Aug 11 2014, 11:09 PM

I've gone full circle on this, Richard suggested resolving 'super' right away. I consumed tokens and annotated the trailing identifier as either type or primary expression. It was so simple and worked perfectly, until dependent bases showed up...

I started hacking and soon enough ended up with something that looks very much like the original patch with few minor changes. NNS now stores the CXXRecordDecl of the class 'super' appeared in, this is replaced by appropriate instantiation in case of templates. This makes us handle the 'derive from template parameter' case just fine, unlike msvc, but I'm fine with it. I guess we could produce an error like them if we wanted but I don't see the point.

I played with printfs to check if the right overload was selected but having tests would be nice. Do I emit IR, dump ast, or to something else?

nikola updated this revision to Diff 12427.Aug 12 2014, 5:23 PM

This patch addresses issues raised by Aaron.

I don't think this could break ObjC, __super is treated as keyword unlike super from ObjC.

nikola updated this revision to Diff 13132.Aug 31 2014, 8:05 PM

Modified tests to cover two issues I found when I ran the patch over our code base at work.

Gentle ping.

rnk added inline comments.Sep 4 2014, 2:34 PM
include/clang/AST/DataRecursiveASTVisitor.h
627

I would drop the "Ms" prefix, and just have a NestedNameSpecifier::Super enumerator. If gcc adds an incompatible super-like NNS we can cross that bridge when we get there.

include/clang/Sema/Sema.h
2611

"LookupInSuper"

4493

"ActOnSuperSpecifier"

lib/AST/ASTContext.cpp
4199

The super specifier isn't unique. I think we can canonicalize them similarly to how namespace specifiers are canonicalized: get the original CXXRecordDecl.

Unfortunately, I'm not sure how to test this.

lib/AST/ItaniumMangle.cpp
1462

This should be an assertion. We can't mangle __super, it is dependent.

lib/Sema/SemaCXXScopeSpec.cpp
256–259

I wonder if there is a better way to find the enclosing class scope. What about this test case:

struct B { typedef int T; };
struct A : B { enum C { X = sizeof(__super::T) }; };
test/SemaCXX/MicrosoftExtensions.cpp
404

"with a using declaration" :)

512

Can you add some tests with more nested DeclContexts? enums form a DeclContext, so those are handy and can go inside records and functions.

You can also use inner classes like:

struct Base { typedef int T; };
struct Outer { struct Inner : Base { static const int x = sizeof(__super::T); }; };

I bet __super also misbehaves inside a lambda. I won't insist on actually handling this case in this patch, but we should at least have test coverage that shows we don't crash.

nikola added inline comments.Sep 16 2014, 8:34 PM
lib/Sema/SemaCXXScopeSpec.cpp
256–259

You're right, but instead of handling this case I'd like to turn the assert bellow into an error. That will handle the lambdas as well. What do you think? This seems like an abuse to me :)

test/SemaCXX/MicrosoftExtensions.cpp
512

This example works just fine, I'll add it to existing tests.

We're not crashing on lambdas but I'll add the tests. Currently we error out because lambda has no base classes but hopefully we can fix this by doing what I suggested in the previous comment.

msvc is so funny:

void foo() {
  []{ __super::bar(); };
}

will fail with: 'super' : this keyword can only be used within the body of class member

struct A {
  void foo() {
    []{ __super::bar(); };
  }
};

will fail with: illegal use of 'super': 'A' does not have any base classes

Adding a base class will change the error to: illegal use of 'super': 'A::foo::lambda_hash>' does not have any base classes

Have I missed any cases?

rnk added inline comments.Sep 17 2014, 5:38 PM
lib/Sema/SemaCXXScopeSpec.cpp
256–259

I think your Scope check will actually break on simple things like:

struct B { typedef int T; };
struct A : B { void f() { if (int x = 1) { return sizeof(__super::T); } } };

Scopes have a fairly close mapping to curly braces in the source level.

I think you can use something like:

CXXRecordDecl *RD = nullptr;
for (const Scope *S = getCurScope(); S; S = S->getParent()) {
  if (S->isFnScope()) {
    MD = dyn_cast<CXXMethodDecl>(S->getEntity());
    if (MD) RD = MD->getParent();
    break;
  } else if (S->isClassScope()) {
    RD = cast<CXXRecordDecl>(S->getEntity());
    break;
  }
}
if (!RD) error
test/SemaCXX/MicrosoftExtensions.cpp
512

Nope, sounds good.

nikola updated this revision to Diff 14058.Sep 24 2014, 6:40 PM

Fingers crossed.

I'm not too happy with what I had to do with FileCheck to get the lambda test case working. This whole file expects c++11 to be disabled which prevents me from using -verify. I thought about moving this to MicrosoftCompatibility.cpp but that's misleading as super is controlled by -mfs-extensions. Separate file for super tests would be the way to solve all problems?

rnk accepted this revision.Sep 25 2014, 9:36 AM
rnk edited edge metadata.

lgtm

Please commit, this looks pretty good. If you want to make any more substantial changes I'd rather see a new patch. Thanks for the feature!

In D4468#15, @nikola wrote:

Fingers crossed.

I'm not too happy with what I had to do with FileCheck to get the lambda test case working. This whole file expects c++11 to be disabled which prevents me from using -verify. I thought about moving this to MicrosoftCompatibility.cpp but that's misleading as super is controlled by -mfs-extensions. Separate file for super tests would be the way to solve all problems?

I don't see the FileCheck hacks in this patch. Did you add the file? That said, feel free to split the __super test cases out. MicrosoftExtensions.cpp dates back to a simpler time when there were fewer extensions. :)

lib/Sema/SemaCXXScopeSpec.cpp
259

Why can't we use this in static member functions? It appears to work with MSVC 2013, and MSDN just says "can only appear inside a member function". This prints B::f:

extern "C" void puts(const char *);
struct B {
  static void f() {
    puts("B::f");
  }
};
struct A : B {
  static void f() {
    __super::f();
  }
};
int main() {
  A::f();
}
269

I think we should give a custom diagnostic for lambdas. The diagnostic we issue currently will confuse users, because it will appear that they are using __super from a method, and we will say they aren't.

lib/Sema/TreeTransform.h
3106

Use Q.getBeginLoc() instead of no SLoc?

test/SemaCXX/MicrosoftExtensions.cpp
407–409

Ouch, this is bad error recovery. It'd be nice to figure out why we can recover from 'Undeclared::foo();' with less spew.

This revision is now accepted and ready to land.Sep 25 2014, 9:36 AM
nikola closed this revision.Sep 25 2014, 5:42 PM

Done in r218484. Thanks for the review and all suggestions.

Last patch was not the final version, but something I uploaded by mistake. I've allowed use from static members, added lambda diagnostic and moved test to a separate file. The only thing left would be to improve error recovery, I'll check this when I get the time.