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 #137920)

Opening brace should follow the closing paren on previous line.

2963 ↗(On Diff #137920)

Use early exit here:

if (!Seq)

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

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

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

Use early exit here:

if (!isa<CXXConstructorDecl>(FD)

// old if-block code

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 #137920)

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.


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.