remove copying when invoking objects stored in sbo #24
Conversation
|
Instead of |
I'm trying that but I'm having trouble getting code like this to then compile: move_only_function<int( noex_callable ) const> f2( std::move( f1 ) );
BOOST_TEST_EQ( f2( c ), 1235 );
move_only_function<int( noex_callable )> f3( std::move( f2 ) );
BOOST_TEST_EQ( f3( c ), 1235 );The compilers reject it with: The problem being there's no conversion from: int (*)(const boost::compat::detail::storage&to int (*)(boost::compat::detail::storage&Many of the tests fail like: Do you want me to push up my WIP branch? The more I think about it, the const_cast is probably harmless here anyway because we correctly apply the cv-ref qualifiers at the invoke site. I'm not sure what we should do to fix these kinds of function pointer cast errors. |
|
Ah, I see. Fewer const_casts will be needed if you declare the functions to take |
cmazakas
force-pushed
the
fix/sbo-invoke-copy
branch
2 times, most recently
from
eed8de3 to
d9faeab
Compare
| #endif | ||
|
|
||
| #ifdef _MSC_VER | ||
| #pragma warning(disable: 4789) // false buffer overrun warning in test_mutable_lambda() |
There was a problem hiding this comment.
What is the buffer overrun warning?
|
Can you please structure this as: commit 1 - test additions without the 4789 warning suppression; commit 2 - changes to the implementation; commit 3 - warning suppression in the test? |
This prevents the code from erroneously copying the storage which causes surprising behavior as noted here: boostorg#23
The msvc optimizer runs into a bug in the mutable lambda tests for
move_only_function where it believes we're placing a large object into
the small buffer, resulting in compiler errors:
D:\a\compat\boost-root\boost\compat\move_only_function.hpp(400) :
error C2220: the following warning is treated as an error
D:\a\compat\boost-root\boost\compat\move_only_function.hpp(400) :
warning C4789: in function 'void __cdecl test_mutable_lambda(void)'
buffer 'func2' of size 32 bytes will be overrun; 256 bytes will be
written starting at offset 0
The code, however, is correct and eschews the SBO path for sufficiently
large Callables, as if one manually adds a `throw 1234;` statement to
the SBO paths, they are not hit when the test case in question is run.
We choose to manually suppress the warning as no other compiler emits it
nor is it found by any of our sanitizer CI jobs, including locally.
cmazakas
force-pushed
the
fix/sbo-invoke-copy
branch
from
d9faeab to
01d7279
Compare
|
I've merged the first two commits to develop. As for the third one, the code is correct, the branch is never taken. However, there's still a better fix because the reason for the warning is that the compile-time constant condition
A drive-by comment: Best practices when using placement new is to qualify it, |
2 participants
Resolves: #23
Right now the invoke holders take the storage by-value which is strictly incorrect when using SBO as it creates a copy of the callable which leads to surprising results in user-code that expect multiple calls to a lambda with a mutable capture.