Page MenuHomePhabricator

[Sema] Implement C++14's DR1579: Prefer moving id-expression out of functions
ClosedPublic

Authored by erik.pilkington on Jun 22 2016, 1:23 PM.

Details

Summary

DR1579 says that when returning a id-expression from a function, we should attempt to return said expression by move first, then fallback to copy.

This patch does this by generalizing PerformMoveOrCopyElision, which previously did this when returning a id-expression that was a parameter.

This patch allows the following code to compile, for example:

struct Base {};
struct Derived : public Base {};
std::unique_ptr<Base> f() {
  std::unique_ptr<Derived> d;
  return d;
}

The DR in question: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579
Pointed out by PR27785.

Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

erik.pilkington retitled this revision from to [Sema] Implement C++14's DR1579: Prefer moving id-expression out of functions.
erik.pilkington updated this object.
erik.pilkington added reviewers: rsmith, faisalv.
erik.pilkington added a subscriber: cfe-commits.
EricWF added a subscriber: EricWF.Jun 22 2016, 2:24 PM
rsmith edited edge metadata.Jun 28 2016, 2:20 PM

Thank you for working on this! Please also add a test to test/CXX/drs/dr15xx.cpp with a "// dr1579: 3.9" comment (we have a script that turns those comments into www/cxx_dr_status.html).

include/clang/Sema/Sema.h
3473 ↗(On Diff #61589)

Constructable -> Constructible.

lib/Sema/SemaStmt.cpp
2756–2759 ↗(On Diff #61589)

Combine these conditions into a single if, and put the (cheapest) bool check first.

2773–2783 ↗(On Diff #61589)

None of this should be checked for the AllowParamOrMoveConstructible case.

2820 ↗(On Diff #61589)

Why are you using direct-initialization here? The return value should always be copy-initialized.

erik.pilkington edited edge metadata.

This new patch addresses all of Richard's comments.

rsmith accepted this revision.Jun 30 2016, 1:50 PM
rsmith edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Jun 30 2016, 1:50 PM
This revision was automatically updated to reflect the committed changes.

Is it also PR23849? Is it dup with PR27785?

Yes, also PR28096. I marked them as being duplicates on the bugzilla.