Page MenuHomePhabricator

Fix for clang PR 29087
ClosedPublic

Authored by twoh on Aug 22 2016, 2:02 AM.

Details

Summary

Since rL274049, for an inheriting constructor declaration, the name of the using declaration (and using shadow declaration comes from the using declaration) is the name of a derived class, not the base class (line 8225-8232 of lib/Sema/SemaDeclCXX.cpp in https://reviews.llvm.org/rL274049). Because of this, name-based lookup performed inside Sema::LookupConstructors returns not only CXXConstructorDecls but also Using(Shadow)Decls, which results assertion failure reported in PR 29087.

Diff Detail

Repository
rL LLVM

Event Timeline

twoh updated this revision to Diff 68832.Aug 22 2016, 2:02 AM
twoh retitled this revision from to Fix for clang PR 29087.
twoh updated this object.
twoh added a reviewer: rsmith.
twoh added a subscriber: cfe-commits.

You need to provide a test for the fix.

twoh updated this revision to Diff 71560.Sep 15 2016, 2:51 PM

Tests added

rsmith added inline comments.Sep 15 2016, 3:14 PM
lib/Sema/SemaExprCXX.cpp
4227 ↗(On Diff #71560)

This isn't really right: a UsingShadowDecl whose underlying declaration is a constructor should itself act like a constructor. This could matter in some obscure cases:

struct B;
struct A { A(B&); };
struct B : A { using A::A; };

What does __has_nothrow_copy(B) return? It should probably be false, since copying a non-const B object will invoke the A(B&) constructor, which may throw, even though the B(const B&) constructor does not.

However, these __has_* traits should be considered deprecated and are essentially useless, so perhaps it doesn't matter too much whether we get these corner cases right. We also get the constructor template case "wrong" here, which I would imagine comes up a lot more frequently.

test/SemaCXX/crash-has-nothrow-constructor.cpp
1 ↗(On Diff #71560)

Please add these tests into some existing test file for the type traits rather than adding two new files.

twoh updated this revision to Diff 71711.Sep 16 2016, 3:43 PM

Updated diff. For ConstructorUsingShadowDecl, test with its target CXXConstructorDecl, but only when it is not a default/copy/move constructor.

twoh added a comment.Sep 16 2016, 3:48 PM

@rsmith Thank you for your review! I added tests to cxx11-crashes.cpp, as the goal of this patch is not handling __has_* traits right but preventing ICE. Also, I tried to use ConstructorUsingShadowDecl::getConstructor instead of ConstructorUsingShadowDecl::getTargetDecl followed by casting, but clang build was failed with undefined references. I wonder if the definition of ConstructorUsingShadowDecl::getConstructor is missed in https://reviews.llvm.org/rL274049.

sepavloff added inline comments.Oct 21 2016, 4:26 AM
lib/Sema/SemaExprCXX.cpp
4231 ↗(On Diff #71711)

Use auto here. Type of CSD is clear from dyn_cast.

4233 ↗(On Diff #71711)

This assert is excessive. The subsequent cast makes this check.

4239–4240 ↗(On Diff #71711)

Put else on the same line as }.

4281 ↗(On Diff #71711)

Use auto here.

4283 ↗(On Diff #71711)

The assert is excessive.

4289–4290 ↗(On Diff #71711)

Put else on the same line as }.

twoh updated this revision to Diff 75441.Oct 21 2016, 9:14 AM
twoh marked an inline comment as done.

Addressing comments from @sepavloff. Thanks!

twoh marked an inline comment as not done.Oct 28 2016, 6:11 PM

Ping

rsmith added inline comments.Oct 29 2016, 11:01 AM
lib/Sema/SemaExprCXX.cpp
4390–4391 ↗(On Diff #75441)

You also need to handle the case of an inherited constructor template (isa<FunctionTemplateDecl>(ND->getUnderlyingDecl()) will do the right thing in both cases).

4395–4397 ↗(On Diff #75441)

This is better written as

auto *Constructor = cast<CXXConstructorDecl>(ND->getUnderlyingDecl());
4398–4401 ↗(On Diff #75441)

This is not necessary: Sema has already filtered out the ones that aren't inherited.

twoh updated this revision to Diff 76817.Nov 2 2016, 11:49 PM

Addressing comments from @rsmith. Thanks!

rsmith accepted this revision.Nov 23 2016, 1:35 PM
rsmith edited edge metadata.

Looks good for trunk and 3.9 branch.

This revision is now accepted and ready to land.Nov 23 2016, 1:35 PM
This revision was automatically updated to reflect the committed changes.