Rewrite MFBT utils with variadic template

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Posted patch patch 1 - Maybe (obsolete) — Splinter Review
Attachment #8545880 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Summary: Rewrite Maybe::emplace with variadic template → Rewrite MFBT utils with variadic template
(Assignee)

Comment 2

4 years ago
Posted patch patch 2 - MaybeOneOf (obsolete) — Splinter Review
Attachment #8545885 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

4 years ago
Attachment #8545886 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Attachment #8545880 - Attachment description: patch → patch 1 - Maybe
(Assignee)

Comment 4

4 years ago
try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26f9c2b0db0

seems it works fine on all platforms.
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 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

4 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

4 years ago
Attachment #8545880 - Attachment is obsolete: true
Attachment #8546549 - Flags: review?(jwalden+bmo)
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 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

4 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

4 years ago
One more file ;)
Attachment #8547009 - Flags: review?(jwalden+bmo)
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+
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+
(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.
Better, but still gratuitous verbosity far beyond a fault.
Note this is likely to break b2g builds for the same reason bug 1120620 and bug 1120622 had to be backed out.
Mmm or maybe not, cf. your try. We'll see.
You need to log in before you can comment on or make changes to this bug.