On 8/12/19 10:38 PM, Bryce Seager van Dyk wrote:
On Monday, August 12, 2019 at 8:41:26 AM UTC-7, Emilio Cobos Álvarez wrote:
Neat! Thanks for doing this. It should allow for broader use of Result in media
code.
Are there any footguns to watch out for when moving into the Result? Looking at the
changes it appears that if my return type is Result<BigStruct> then I can just
return a BigStruct? I.e. I don't need to do explicitly invoke anything else to avoid
unintended copies.
I think you need to use `return std::move(bigStruct);`, since otherwise
you peek the `const T&` constructor rather than the `T&&` constructor.
Before my patch, you always get a copy when returning it (because Result
only took const references).
With my patch, if you `return bigStruct;`, your code will compile and do
the same copy, but you'll get a warning from the compiler with
-Wreturn-std-move, such as:
> In file included from
z:/build/build/src/obj-firefox/toolkit/mozapps/extensions/Unified_cpp_mozapps_extensions0.cpp:11:
>
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10):
error: local variable 'result' will be copied despite being returned by
name [-Werror,-Wreturn-std-move]
> return result;
> ^~~~~~
>
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10):
note: call 'std::move' explicitly to avoid copying
> return result;
> ^~~~~~
> std::move(result)
>
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10):
error: local variable 'result' will be copied despite being returned by
name [-Werror,-Wreturn-std-move]
> return result;
> ^~~~~~
>
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(554,20):
note: in instantiation of function template specialization
'mozilla::EncodeLZ4<char [16]>' requested here
> MOZ_TRY_VAR(lz4, EncodeLZ4(scData, STRUCTURED_CLONE_MAGIC));
> ^
>
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10):
note: call 'std::move' explicitly to avoid copying
> return result;
> ^~~~~~
> std::move(result)
Note that this does not -Werror everywhere on automation right now[1],
but I think we should make it so, per that LLVM bug.
But anyhow the compiler will tell you to do the fast thing once my patch
lands.
-- Emilio
[1]:
https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/build/moz.configure/warnings.configure#144
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
