Page MenuHomePhabricator

[Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.
ClosedPublic

Authored by comex on Sep 18 2019, 5:35 PM.

Details

Summary

Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs.

Note that for by-value arguments, the object being consumed is always a temporary. If you write:

void a(X); a((X &&)x);

...this first constructs a temporary X by moving from x, then calls a with a pointer to the temporary. Before and after this change, the move copies x's typestate to the temporary and marks x itself as consumed. But before this change, if the temporary started out unconsumed (because x was unconsumed before the move), it would still be unconsumed when its destructor was run after the call. Now it will be considered consumed at that point.

Also note that currently, only parameters explicitly marked return_typestate have their state-on-return checked on the callee side. Parameters which are implicitly consuming due to being rvalue references – or, after this patch, by-value – are checked only on the caller side. This discrepancy was very surprising to me as a user. I wrote a fix, but in my codebase it broke some code that was using std::forward. Therefore, I plan to hold off on submitting the fix until a followup patch, which will generalize the current std::move special case into an attribute that can be applied to any 'cast' function like std::move and std::forward.

Diff Detail

Repository
rL LLVM

Event Timeline

comex created this revision.Sep 18 2019, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 5:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dblaikie accepted this revision.Sep 19 2019, 12:32 PM

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the design implications of that. I have some doubts about supporting non-consumed after passing by rvalue ref, but don't feel too strongly & the alternative would be having a warning about the attribute being ignored, etc - when it has a fairly clear meaning if it is there, just not sure people 'should' be doing that.

test/SemaCXX/warn-consumed-analysis.cpp
62–64 ↗(On Diff #220778)

unless these are testing something noteworthy about them being static functions I'd probably suggest making them non-members like the other test functions below, for consistency (otherwise the inconsistency tends to raise the question of "what is significant about this difference?")

This revision is now accepted and ready to land.Sep 19 2019, 12:32 PM

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the design implications of that. I have some doubts about supporting non-consumed after passing by rvalue ref, but don't feel too strongly & the alternative would be having a warning about the attribute being ignored, etc - when it has a fairly clear meaning if it is there, just not sure people 'should' be doing that.

Fair enough. I agree there's not much reason to do that, but it also seems pretty harmless to respect the user's choice. Silently ignoring the attribute as before is obviously wrong, so the alternative would be adding a diagnostic for that case, but it doesn't seem worth it...

unless these are testing something noteworthy about them being static functions I'd probably suggest making them non-members like the other test functions below, for consistency (otherwise the inconsistency tends to raise the question of "what is significant about this difference?")

Okay, I'll change them to non-members before committing.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2019, 4:02 PM