Page MenuHomePhabricator

Only use MSVC-compatible dependent base class hack for class template members.

Authored by kimgr on Aug 11 2014, 2:56 PM.



Currently the code in SemaCXXScopeSpec does an MSVCCompat fallback to delay lookup of possibly dependent base class names. It does so in all template bodies, even if the template is a simple function template.

Attached patch conditionalizes the compat fallback on whether the template function is part of a CXXRecordDecl as well, so that the dependent base class lookup workaround only happens when there's a chance of dependent base classes.

See discussion here:

Diff Detail


Event Timeline

kimgr updated this revision to Diff 12367.Aug 11 2014, 2:56 PM
kimgr retitled this revision from to Only use MSVC-compatible dependent base class hack for class template members..
kimgr updated this object.
kimgr edited the test plan for this revision. (Show Details)
kimgr added a reviewer: rnk.
kimgr added a subscriber: Unknown Object (MLST).
kimgr updated this object.Aug 11 2014, 2:58 PM

I've played with another approach, to just ask the DeclContext if its parent is a record, i.e.

if (DC->isDependentContext() && DC->isFunctionOrMethod() && DC->getParent()->isRecord()) {
   // ...

and it seems to work equally well for the test cases I've provided.

I'm not sure if checking for records is sufficient, though, I'm worried about constructs like:

struct Foo {
  template<class T>
  void bar() {

Does that even qualify as a dependent base lookup?


rnk accepted this revision.Aug 13 2014, 10:07 AM
rnk edited edge metadata.


This isn't consistent with MSVC, but it doesn't break ATL "hello world" at the very least. If we commit this, I'll find out what happens in Chrome overnight. Do you need me to land it?

This revision is now accepted and ready to land.Aug 13 2014, 10:07 AM
kimgr updated this revision to Diff 12469.Aug 13 2014, 1:30 PM
kimgr edited edge metadata.

Thanks! Could you try this one instead? I think it's more representative of what we should do, i.e. only fall back if we're in a CXXRecordDecl and said record has dependent bases.

kimgr added a comment.Aug 13 2014, 1:32 PM

... and by 'should', I mean what I intended, to avoid this fallback when the chance of dependent bases is zero.

I don't have commit access, so I'd love it if you could land this, and Chromium is a great baseline, so thanks for trying that!

rnk closed this revision.Aug 14 2014, 4:44 PM
rnk updated this revision to Diff 12536.

Closed by commit rL215683 (authored by @rnk).

rnk added a comment.Aug 14 2014, 4:45 PM

I landed this in r215683 with a warning and some tests for it. The diagnostic leaves a bit to be desired, though.

Thanks for the patch!