Page MenuHomePhabricator

Delay lookup of simple default template arguments under -fms-compatibility
ClosedPublic

Authored by rnk on Jun 2 2014, 3:49 PM.

Details

Summary

MSVC delays parsing of default arguments until instantiation. If the
default argument is never used, it is never parsed. We don't model
this.

Instead, if lookup of a type name fails in a template argument context,
we form a DependentNameType, which will be looked up at instantiation
time.

This fixes errors about 'CControlWinTraits' in atlwin.h.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 10029.Jun 2 2014, 3:49 PM
rnk retitled this revision from to Delay lookup of simple default template arguments under -fms-compatibility.
rnk updated this object.
rnk added a reviewer: rsmith.
rnk added a subscriber: Unknown Object (MLST).
majnemer added inline comments.
lib/Parse/ParseDecl.cpp
2764 ↗(On Diff #10029)

Perhaps move the DSContext == DSC_template_type_arg check before the calls to getLangOpts() ? It should be a little cheaper.

lib/Sema/SemaDecl.cpp
358 ↗(On Diff #10029)

Perhaps auto RD = dyn_cast<CXXRecordDecl(DC)) ?

366 ↗(On Diff #10029)

Perhaps a comment as to why we are emitting this diagnostic?

rsmith added inline comments.Jun 2 2014, 5:12 PM
include/clang/AST/Type.h
4059–4061 ↗(On Diff #10029)

Please update this comment to indicate we also use DependentNameType in cases where we know we have a type but wish to defer the lookup for MS compatibility.

lib/Parse/ParseDecl.cpp
2764 ↗(On Diff #10029)

I would think the most common reason this if condition is false is because MSVCCompat is false =) But I don't care either way.

lib/Sema/SemaDecl.cpp
353–354 ↗(On Diff #10029)

Hmm. I don't think we actually ever need to recurse. Just walk up to the context into which we should look up the name, and build a single-level NNS for that.

358–359 ↗(On Diff #10029)

Should we really ever use a class as the nested name specifier here? If I have a nested class (template) inside a class (template), should I look up a missing base specifier type in the outer class, or in the innermost enclosing namespace? (I would expect the latter is the right answer.)

361 ↗(On Diff #10029)

Once we hit this, we should bail out entirely. (This can currently include local classes in the name specifier, which doesn't seem right.)

test/SemaTemplate/ms-delayed-default-template-args.cpp
6–9 ↗(On Diff #10029)

Maybe also add a test for

template<typename T> struct Something {};
template<typename T = Something<Bar>> struct SomethingElse;

... and

template<Something<Bar> *p> struct AnotherThing;
rnk updated this revision to Diff 10059.Jun 3 2014, 1:38 PM
  • Add tests and fix nits
include/clang/AST/Type.h
4059–4061 ↗(On Diff #10029)

Done

lib/Parse/ParseDecl.cpp
2764 ↗(On Diff #10029)

OK, I'll hoist the DSC check. I don't think we fail name lookup in template argument contexts very often, but I could be wrong.

lib/Sema/SemaDecl.cpp
353–354 ↗(On Diff #10029)

OK.

358 ↗(On Diff #10029)

done

358–359 ↗(On Diff #10029)

We need to be able to do lookup in enclosing RecordDecl scopes. MSVC accepts this:

struct Outer {
  template <typename T = Baz> struct Foo {
    static_assert(sizeof(T) == 4, "should get int, not double");
  };
  typedef int Baz;
};
typedef double Baz;
template struct Outer::Foo<>;
361 ↗(On Diff #10029)

Bail out entirely, as in, return nullptr, or as in, fail the lookup?

366 ↗(On Diff #10029)

done

test/SemaTemplate/ms-delayed-default-template-args.cpp
6–9 ↗(On Diff #10029)

I assume Bar is always undeclared in these tests.

MSVC accepts your first test case, but Clang doesn't with my patch. I'd need to find a way to delay instantiating Something<Bar> until later, which feels beyond the scope of this patch.

The second test is a negative test, right? Both we and MSVC need to know the type of the non-type template parameter.

rsmith added inline comments.Jun 5 2014, 4:50 PM
include/clang/AST/Type.h
4056–4057 ↗(On Diff #10059)

You seem to be using this to represent an unqualified type in some cases? (You've made NNS optional.) A quick look indicates that downstream code will not do the right thing in this case. (TreeTransform will treat it as if an error has occurred, and bail out with no diagnostic, for instance.)

lib/Sema/SemaDecl.cpp
361 ↗(On Diff #10029)

I think this has been made irrelevant by other changes.

353–357 ↗(On Diff #10059)

What about the translation unit? Should you create a :: NestedNameSpecifier for it? (Once you do that, the 'return nullptr;' below and the null NNS case in DependentNameType are unreachable.)

test/SemaTemplate/ms-delayed-default-template-args.cpp
6–9 ↗(On Diff #10029)

I had meant for these to both be negative tests (maybe with a FIXME for the first case).

rnk added inline comments.Jun 5 2014, 6:06 PM
include/clang/AST/Type.h
4056–4057 ↗(On Diff #10059)

This should be fixed now by using GlobalSpecifier. I guess I have tests covering this case, but we would silently have an invalid AST, so they didn't fail.

lib/Sema/SemaDecl.cpp
353–357 ↗(On Diff #10059)

OK, that works. :)

test/SemaTemplate/ms-delayed-default-template-args.cpp
6–9 ↗(On Diff #10029)

OK, good. :)

rnk updated this revision to Diff 10162.Jun 5 2014, 6:06 PM
  • Add tests and fix nits
  • Use GlobalSpecifier so the delayed dependent name is always qualified
rsmith accepted this revision.Jun 5 2014, 6:26 PM
rsmith edited edge metadata.

LGTM

lib/Sema/SemaDecl.cpp
349 ↗(On Diff #10162)

This looks redundant.

This revision is now accepted and ready to land.Jun 5 2014, 6:26 PM
rnk closed this revision.Jun 6 2014, 3:44 PM
rnk updated this revision to Diff 10192.

Closed by commit rL210382 (authored by @rnk).

rnk added a comment.Jun 6 2014, 3:44 PM

Thanks for the help and the review! r210382