Page MenuHomePhabricator

Preliminary refactoring in service of -Wreturn-std-move. NFC.

Authored by Quuxplusone on Feb 28 2018, 1:14 PM.



This patch is extracted from D43322, which adds a new diagnostic -Wreturn-std-move. This patch here is just the non-functional parts of that patch. It pulls TryMoveInitialization out into a separate function so that we can (in the next patch) try it twice — once with current C++ rules, and once with the rules as they would be if return x considered rvalue-ref-qualified conversion operators. This patch here does *not* add those new rules; it merely rearranges the existing code to make the next patch less bulky.

Diff Detail


Event Timeline

Quuxplusone created this revision.Feb 28 2018, 1:14 PM

Add a block comment for function TryMoveInitialization.

@rtrieu please take a look?

Also, I would need someone to commit this for me, as I don't have commit privs.

rtrieu added inline comments.Mar 9 2018, 5:01 PM
2953 ↗(On Diff #136380)

Opening brace should follow the closing paren on previous line.

2963 ↗(On Diff #136380)

Use early exit here:

if (!Seq)

// rest of function
2965–2966 ↗(On Diff #136380)

Since you're simplifying the condition here, bring the not operator inside the parentheses.

if (Step.Kind != ...  && Step.Kind != ...)
2970 ↗(On Diff #136380)

Use early exit here:

if (!isa<CXXConstructorDecl>(FD)

// old if-block code
2999–3000 ↗(On Diff #136380)

At this point, the variable Value is updated. Value is scoped to this function, and used again after this code. In your change, there is a new Value variable in the static function. Only that variable is updated and not this one, making this a change in behavior.

Quuxplusone marked 4 inline comments as done.Mar 10 2018, 11:54 AM
Quuxplusone added inline comments.
2970 ↗(On Diff #136380)

I'd prefer not to do this, since D43322 is going to change this code into "if isa-red-fish... else if isa-blue-fish...". Therefore I think it makes sense to keep this intermediate stage as "if isa-red-fish...", rather than changing it into "if not-isa-red-fish continue... otherwise..."

If you really want this, I can change it; but it's just going to change back in D43322, and the goal of this patch was to make D43322 smaller.

2999–3000 ↗(On Diff #136380)

Good catch! I've addressed this now by making the parameter Expr *&Value; but I'd be open to better approaches. Particularly because I still don't know what to do about the "unnecessary promoting Value to the heap" that will happen in D43322.

Quuxplusone marked an inline comment as done.

Addressed @rtrieu's comments.

@rtrieu, please take another look at this and D43322? Thanks!

Everything looks good and ready for commit.

@rtrieu thanks! I don't have commit privileges; could I ask you to commit this on my behalf? (Or if not, please say no, and I'll know to go looking for someone else to ask.)

Once/if D43898 hits master, I'll rebase D43322 and await a LGTM there, at which point I'll again ask for help committing it. :)

This revision was not accepted when it landed; it landed in state Needs Review.Mar 14 2018, 8:06 PM
This revision was automatically updated to reflect the committed changes.