This is an archive of the discontinued LLVM Phabricator instance.

[MSVC] Better lookup into dependent bases for unspecified declaration (also fixes http://llvm.org/PR22066)
AbandonedPublic

Authored by ABataev on Mar 5 2015, 11:28 PM.

Details

Reviewers
thakis
rnk
rsmith
Summary

Provides better qualified/unqualified lookup into dependent bases for MSVC code + improves diagnostics.

Diff Detail

Event Timeline

ABataev updated this revision to Diff 21334.Mar 5 2015, 11:28 PM
ABataev retitled this revision from to [MSVC] Better lookup into dependent bases for unspecified declaration (also fixes http://llvm.org/PR2206).
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added reviewers: rsmith, rnk, thakis.
ABataev added a subscriber: Unknown Object (MLST).
ABataev retitled this revision from [MSVC] Better lookup into dependent bases for unspecified declaration (also fixes http://llvm.org/PR2206) to [MSVC] Better lookup into dependent bases for unspecified declaration (also fixes http://llvm.org/PR22066).Mar 6 2015, 8:46 AM
rnk edited edge metadata.Mar 9 2015, 4:36 PM

I'm still not 100% sure what's going on here. In particular, I'd like to better understand the meaning of IdNameLookupResult.

Do you think you can split out a small change that adds the "insert typename here" suggestions? That would be a mostly mechanical change to update test results that significantly reduces the diff.

include/clang/Sema/Sema.h
3518

Nobody in Parse calls this, so this can be QualType instead of ParsedType.

3519

In this change, all call sites pass all of these parameters. Can you drop the default values?

lib/Sema/SemaDecl.cpp
150–161

This needs editing. I think you might be better off documenting each IdNameLookupResult individually.

155

I think we can use a simple function pointer without using too much power. Alternatively, llvm::function_ref is lighter weight.

263–270

Looks like you can hoist the NonInstanceMemberOnly check out, and then remove the need for std::function. Consider:

if (NonInstanceMemberOnly)
  return isDeclInDependentClass(
      *this, SS, Loc, II, SupposeExistInBaseParm, [](NamedDecl *ND) {
        return ND->isCXXClassMember() && !ND->isCXXInstanceMember();
      });
return isDeclInDependentClass(
    *this, SS, Loc, II, SupposeExistInBaseParm,
    [](NamedDecl *ND) { return ND->isCXXClassMember(); });
lib/Sema/SemaExpr.cpp
1959

These conditions are equivalent, right? This can be if (ObjectType)

1989

Ditto, !ObjectType && SS && SS->isEmpty()

2105–2123

No need to use doxygen style /// comments in a function

Reid, I will remove all 'typename' related code.

include/clang/Sema/Sema.h
3518

Fixed

3519

Ok

lib/Sema/SemaDecl.cpp
150–161

Added an additional comment about SupposeFoundSpecifiedKind element and added comments to IdNameLookupResult enum and its elements.

155

Changed it to function_ref

263–270

Ok, consider it done

lib/Sema/SemaExpr.cpp
1959

Fixed

1989

Fixed

2105–2123

Ok, fixed

ABataev updated this revision to Diff 21566.Mar 10 2015, 4:59 AM
ABataev edited edge metadata.

Update after review

rnk added a comment.Mar 16 2015, 1:59 PM

I think this is pretty close to done, but I want to get an opinion from Richard before committing, since he's more familiar with lookup.

lib/Sema/SemaDecl.cpp
139–142

OK, this is sort of the case being described:

template <typename T> struct Foo : T { void f(int a) { IdFromBase(a); } };

I think we need better terminology to talk about how this case differs from this case:

template <typename T> struct Foo : Bar<T> { ... };

Both of these are classes with dependent bases, but in the second case, we can go searching through the template pattern for Bar to try to see if it has type or non-type members. I think if we can come up with a name for one of these two cases, we can make this code a lot clearer.

We should get Richard's input on this, but here are some ideas:

  • Fully dependent / partially dependent
  • Dependent base with known pattern / unknown dependent base

So far, I like using the "known / unknown pattern" terminology best, so I might rename this enumerator to NotFoundWithUnknownBase. This indicates that after doing extended lookup into dependent bases with known patterns, no members with this name were found, but we hit an unknown base that we couldn't look through.

157

If we go with this "known / unknown" terminology, this can be lookupIdInKnownDependentClass or something.

170

This can be an early return:

if (FoundDecl != IdNameLookupResult::NotFound)
  return FoundDecl;
195

This can also continue early:

if (!BaseRD)
  continue;
220

This is still std::function

Reid, thanks for the review!

lib/Sema/SemaDecl.cpp
139–142

Yep, you're right. I also prefer "known / unknown pattern". I'll rename the enumerator.

170

Ok, fixed

195

Fixed

220

Fixed

ABataev updated this revision to Diff 22074.Mar 17 2015, 12:36 AM

Update after review

rsmith added inline comments.Mar 17 2015, 5:23 PM
include/clang/Sema/Sema.h
3500–3518

From reading this alone, I have very little idea what this function does.

lib/Sema/SemaDecl.cpp
137–144

I don't think you mean 'type' here. Also remove the 'If'.

139

Remove 'is'.

140

Why only of that type? Why not any dependent type that we can't heuristically look into?

143

Again, 'type' seems wrong here. 'kind' perhaps?

148–150

How about something like "Attempt to determine what lookup of a name would find in an instantiation of a dependent class. For a dependent base class, we make a heuristic guess as to the template from which the base class will be instantiated."

157

IdName doesn't really mean anything. Drop the Id here? Also, this name suggests that we're doing a normal lookup into a dependent class, following standard rules, and producing a list of declarations, which isn't correct.

Something like classifyNameInKnownDependentBases might be a better name.

158

This interface would make more sense if it took a DeclarationName rather than a const IdentifierInfo &.

159

Maybe something like AssumeExistsInUnknownDependentBase would be clearer than SupposeExistInBaseParm? But this flag seems to be unnecessary -- instead, the callers can distinguish between NotFound and NotFoundWithUnknownBase, or not, as necessary.

163–164

The getCanonicalType call here appears to be redundant.

163–164

Comment seems out of date.

164–165

This seems like a strange case to treat specially. Why would you handle a base class of type T here but not a base class of type T::type? (Why don't we do this for all dependent base classes that we can't heuristically perform lookup into?)

165

You should probably use ND->getUnderlyingDecl() here to handle using-declarations.

165–166

If we find a type and a non-type, the type should win, per [basic.scope.declarative]p4 bullet 2.

218

Again, "dependent class" isn't the right term here, because normal lookup in a class template definition looks for names in a dependent class.

218

Please keep the parameter order consistent between this class and the previous one. Putting the class name / scope specifier before the name being looked up seems like the better order.

225–226

This doesn't look right. If I have

template<typename T> struct X { typedef T type; };

then lookup of type within X<T*>:: should presumably look in the primary template for X rather than looking in the current context.

228

This should be DC->getLookupParent().

232

This requires RD to be the pattern of a class template; I would think that class template partial specializations, member classes of class templates, and local classes within function templates should get the same handling. You can just check RD->isDependentContext() to catch all of those cases.

238–244

It's very strange for a function that looks like a simple predicate to modify its arguments in-place like this. How about making this function return a NestedNameSpecifier * or CXXRecordDecl * instead?

251

Can we somehow suggest in this name that the answer is uncertain if we don't know what all the dependent bases look like? isClassTemplateInKnownDependentBase perhaps?

255

Should we allow other type templates, such as AliasTemplateDecl here too?

287–292

Why don't we emit a diagnostic here for qualified lookup?

393–394

Comment seems out of date.

826–827

Comment seems out of date.

lib/Sema/SemaExpr.cpp
1959

Please remove 'cxx' from the comments here.

1988–1993

A function named is... should not be mutating state and producing diagnostics like this. Can you move the actual recovery code into the caller, or rename this function to reflect what it does (recoverFromUnknownMemberLookup or something)?

2105–2122

"class template", not "template class", "base class" not "parent class". Also, don't you mean "if we're inside a class template that has dependent base classes"?

2112–2122

The checking performed here should depend on whether this is available. When walking the current class and its base classes, you're allowed to find members that need this and members that don't, but when you recurse to surrounding scopes, you should stop allowing members that need this.

lib/Sema/SemaTemplate.cpp
184–186

This is a weird fixit: it's not syntactically vaild to put typename there. Do we really suggest that?

test/SemaCXX/MicrosoftCompatibility.cpp
169–176

Why do we no longer give these warnings?

test/SemaTemplate/typename-specifier.cpp
214–216

Shouldn't there be a warning here for the MSVC case?

ABataev abandoned this revision.Oct 12 2016, 7:52 AM