PeterSommerlad / SC22WG21_Papers

My submissions to SC22WG21 C++ ISO/IEC standardization group
43 stars 13 forks source link

scope_exit "DemonstrateSurprisingReturnedFromBehavior" test fails with GCC 12 #16

Closed jwakely closed 2 years ago

jwakely commented 2 years ago

https://github.com/PeterSommerlad/SC22WG21_Papers/blob/8d05e460d3ee3ded986436fd03f51dca15e00d76/workspace/P0052_scope_exit/src/Test.cpp#L85-L88

This test fails. The commented out one passes.

I'm not sure what it's relying on that's not true any more, probably something to do with guaranteed elision happening in more cases. Whatever it is, it was always non-portable to assume a moved-from string will be empty.

PeterSommerlad commented 2 years ago

I think the test depends on ordering of destructors. IIRC, the move on return happened before accessing .size(). However, I guess you are guarded by SSO in the concrete test case from looking at it.

You'd better check the actual provided implementations in my repositories scope11 scope14 scope17 instead of the initial prototype implementation accompanying the paper.

I will check with those repos, if the test case bug is still present.

jwakely commented 2 years ago

Right, the function using scope_exit says:

return (s); // pessimize copy elision to demonstrate effect

But I don't think that actually does anything. The current draft says (emphasis mine):

If the expression in a return (8.7.4) or co_return (8.7.5) statement is a (possibly parenthesized) id- expression that names an implicitly movable entity declared in the body or parameter-declaration-clause of the innermost enclosing function or lambda-expression, or

So we have an SSO string which is not guaranteed to be empty after a move, and an unsuccessful attempt to prevent copy elision so that no move ever happens anyway.

jwakely commented 2 years ago

You'd better check the actual provided implementations in my repositories scope11 scope14 scope17 instead of the initial prototype implementation accompanying the paper.

Thanks, I didn't know about these other repos. I'll take a look.

jwakely commented 2 years ago

https://github.com/PeterSommerlad/scope17/blob/6bc8a67271c36d1db03c72e101dd08580a990b86/cevelop-workspace/scope17/src/Test.cpp#L96

In scope17 that test uses std::move(s). That does prevent elision, unlike (s). It still assumes that moved-from string has zero length, which isn't guaranteed. The test does pass with GCC 12 though, at least when using SSO strings. With COW strings if libstdc++ is configured to use "fully dynamic strings" then the std::string move constructor is just a ref-count increment, and the source object is not empty.

PeterSommerlad commented 2 years ago

I wonder if I should keep this as a potentially failing test, or just as example code when the behavior is surprising due to moved-from state. Will adapt the test case here to stick with the moved.

I changed the test to use std::vector move instead, which shouldn't include bovine behavior, I hope.