Change `Maybe` to use a union for storage
Categories
(Core :: MFBT, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox74 | --- | fixed |
People
(Reporter: jld, Assigned: away)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Apparently it's now possible to use non-POD types in unions, but any operations where the trivial approach won't work are deleted. For example, Chromium's base::Optional does this (with a bunch of specialization magic for cases where the default operations do exist; I'm not clear on whether that's necessary or just an optimization).
If we change mozilla::Maybe to use this approach, that should allow it to lose its MOZ_NON_PARAM annotation, because we'll no longer be relying on an over-aligned char array.
|
||
Comment 1•5 years ago
|
||
This is a half-duplicate of bug 1430702. It's a good idea, someone *touches nose* should do it. Beware the GCC warnings about special member functions in unions that I noted in a comment there, tho -- should be workaroundable with enough macros, just the first naive approach won't be fully adequate.
My personal motivation for doing this is to stop the compiler from inserting stack protectors into tons of functions that wouldn't otherwise have them (arrays on the stack are one of the triggers for stack protection). But others see different benefits, such as being able to remove the MOZ_NON_PARAM annotation. Also this is the approach recommended by https://github.com/CppCon/CppCon2019/tree/master/Presentations/how_to_hold_a_t.
|
||
Updated•4 years ago
|
Thank you David for taking this!
A thought: Since we're now using C++17, would it be worth considering implementing our mozilla::Maybe by using std::optional instead, at least as internal storage? If actually possible, of course.
Presumably the std:: writers should know what works best with their associated compilers.
It may be less "fun", but also less maintenance work later on.
(And similarly, mozilla::Variant could now use std::variant.)
Sorry, I didn't see you already had a patch ready, and it looks nice and small; so you may ignore my std::optional suggestion, as it would be much more work!
Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/084d09bbea70 Use a single-member union as the storage for Maybe r=jwalden
|
||
Comment 6•4 years ago
|
||
| bugherder | ||
|
||
Comment 7•4 years ago
|
||
(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #0)
(with a bunch of specialization magic for cases where the default operations do exist; I'm not clear on whether that's necessary or just an optimization).
It's necessary to preserve characteristics, such that, e.g. std::is_trivially_destructible_v<Maybe<int32>> or std::is_trivially_copyable_v<Maybe<int32>> holds. The former is a necessary precondition to make Maybe<T> a literal type when T is a literal type. The latter e.g. is relevant for various templated stuff selecting implementations/overloads based on that, and also for some static analyses such as clang-tidy's performance-unnecessary-value-param. I think it should be done as follow-up work.
Description
•