Closed
Bug 1119199
Opened 9 years ago
Closed 9 years ago
Rewrite MFBT utils with variadic template
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: xidorn, Assigned: xidorn)
Details
Attachments
(4 files, 2 obsolete files)
5.86 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
7.47 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.74 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
With the minimal requirement of MSVC bumps to 2013, we are now enabled to use variadic template. Maybe::emplace can directly benefit from it.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8545880 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Summary: Rewrite Maybe::emplace with variadic template → Rewrite MFBT utils with variadic template
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8545885 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8545886 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8545880 -
Attachment description: patch → patch 1 - Maybe
Assignee | ||
Comment 4•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26f9c2b0db0 seems it works fine on all platforms.
Comment 5•9 years ago
|
||
Comment on attachment 8545880 [details] [diff] [review] patch 1 - Maybe Review of attachment 8545880 [details] [diff] [review]: ----------------------------------------------------------------- Oh c'mon, you couldn't have found more emplace() code to remove in this patch? :-D
Attachment #8545880 -
Flags: review?(jwalden+bmo) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8545886 [details] [diff] [review] patch 3 - UniquePtr Review of attachment 8545886 [details] [diff] [review]: ----------------------------------------------------------------- I'll look at the other patch tomo^H^Hday once I can research exactly why it is perfect forwarding doesn't work to eliminate the need for both U&& *and* const U& overloading in that patch.
Attachment #8545886 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5) > Comment on attachment 8545880 [details] [diff] [review] > patch 1 - Maybe > > Review of attachment 8545880 [details] [diff] [review]: > ----------------------------------------------------------------- > > Oh c'mon, you couldn't have found more emplace() code to remove in this > patch? :-D Ah... It seems there are lots of... map and apply... ok, I'll update the patch :)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8545880 -
Attachment is obsolete: true
Attachment #8546549 -
Flags: review?(jwalden+bmo)
Comment 9•9 years ago
|
||
Comment on attachment 8545885 [details] [diff] [review] patch 2 - MaybeOneOf Review of attachment 8545885 [details] [diff] [review]: ----------------------------------------------------------------- Hmm. Is it still an r+ if I replace your entire patch with a different approach, then r+ that? :-) ::: mfbt/MaybeOneOf.h @@ +65,4 @@ > } > > + template <class T, class... Args> > + void construct(const Args&... aArgs) I...think this was a case of someone not quite understanding perfect forwarding -- Move wouldn't do the right thing on a const&, so more overloads were added to detect exactly that case. Either that, or the author didn't realize that Forward would fail to forward literal nullptr, and he didn't know to pass nullptr as the actual type of the argument, using a local or static_cast<T*>(nullptr). (You should definitely tryserver my approach to be sure there aren't any such pitfalls lurking.) So I think we can have a single method to replace all the various constructs here currently. template <class T, class... Args> void construct(Args&&... aArgs) { MOZ_ASSERT(state == None); state = Type2State<T>::result; ::new (storage.addr()) T(Forward(aArgs)...); }
Attachment #8545885 -
Flags: review?(jwalden+bmo) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8546549 [details] [diff] [review] patch 1 - Maybe Review of attachment 8546549 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I was being facetious/joking when I asked if that was all the code you could remove -- as if there were any more emplace() code that could be removed, which there wasn't. The previous patch was perfectly fine -- feel free to land it. I figured I was being over-the-top enough it'd be clear, but I guess not. On the other hand, I'm more than willing to review more fake-variadic method removals and replacements with variadic macros, so... ;-) Feel free to keep rampaging through fugly code using more variadic templates! ::: mfbt/Maybe.h @@ +347,5 @@ > * If |isSome()|, runs the provided function and returns the result wrapped > * in a Maybe. If |isNothing()|, returns an empty Maybe value. > */ > + template<typename R, typename... FArgs, typename... Args> > + Maybe<R> map(R(*aFunc)(T&, FArgs...), Args&&... aArgs) Put a space after the R here, for very slightly more readability of a function pointer type. @@ +358,5 @@ > return Maybe<R>(); > } > > + template<typename R, typename... FArgs, typename... Args> > + Maybe<R> map(R(*aFunc)(const T&, FArgs...), Args&&... aArgs) const Space after R, same reason.
Attachment #8546549 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 11•9 years ago
|
||
It seems there were two users which don't fit in the new method.
Attachment #8545885 -
Attachment is obsolete: true
Attachment #8547007 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 12•9 years ago
|
||
One more file ;)
Attachment #8547009 -
Flags: review?(jwalden+bmo)
Comment 13•9 years ago
|
||
Comment on attachment 8547007 [details] [diff] [review] patch 2 - MaybeOneOf Review of attachment 8547007 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, that's pretty bad we got away without Move() in those two cases. Not Bad bad as in C++ misuse issue, it's technically all fine, but the style of that is just horribly misleading. I'm surprised the two mistakes managed to cancel each other out without broken functionality.
Attachment #8547007 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 14•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07f34b8496a1 looks like everything goes well.
Comment 15•9 years ago
|
||
Comment on attachment 8547009 [details] [diff] [review] patch 4 - HashFunctions Review of attachment 8547009 [details] [diff] [review]: ----------------------------------------------------------------- I note in passing that all this code previously, and post-patch, assumes that mozilla::AddToHash, mozilla::HashGeneric, etc. are only defined by this file -- no user specializations or such, such that passing argument by copying might perhaps be incorrect, inefficient, or unideal. It might be worth considering letting people provide specializations for their own types, at some point -- in which case it would probably better to perfectly forward everything. But there's no actual need for it now, as far as I can tell. ::: mfbt/HashFunctions.h @@ +191,2 @@ > MOZ_WARN_UNUSED_RESULT uint32_t > +AddToHash(uint32_t aHash, A aA, Args... aArgs) To my dying day I will hate with an unmatched passion the broken Mozilla style of pointlessly prefixing arguments with "a", resulting in such unreadability as |A aA|.
Attachment #8547009 -
Flags: review?(jwalden+bmo) → review+
Comment 16•9 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15) > To my dying day I will hate with an unmatched passion the broken Mozilla > style of pointlessly prefixing arguments with "a", resulting in such > unreadability as |A aA|. Call it "aArg", then.
Comment 17•9 years ago
|
||
Better, but still gratuitous verbosity far beyond a fault.
Assignee | ||
Comment 18•9 years ago
|
||
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1a96f3a4c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2eb48a20d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/accc4cc9009e https://hg.mozilla.org/integration/mozilla-inbound/rev/428abebd41a3
Comment 19•9 years ago
|
||
Note this is likely to break b2g builds for the same reason bug 1120620 and bug 1120622 had to be backed out.
Comment 20•9 years ago
|
||
Mmm or maybe not, cf. your try. We'll see.
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ea1a96f3a4c0 https://hg.mozilla.org/mozilla-central/rev/aa2eb48a20d9 https://hg.mozilla.org/mozilla-central/rev/accc4cc9009e https://hg.mozilla.org/mozilla-central/rev/428abebd41a3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•