Closed Bug 1119199 Opened 9 years ago Closed 9 years ago

Rewrite MFBT utils with variadic template

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: xidorn, Assigned: xidorn)

Details

Attachments

(4 files, 2 obsolete files)

With the minimal requirement of MSVC bumps to 2013, we are now enabled to use variadic template. Maybe::emplace can directly benefit from it.
Attached patch patch 1 - Maybe (obsolete) — Splinter Review
Attachment #8545880 - Flags: review?(jwalden+bmo)
Summary: Rewrite Maybe::emplace with variadic template → Rewrite MFBT utils with variadic template
Attached patch patch 2 - MaybeOneOf (obsolete) — Splinter Review
Attachment #8545885 - Flags: review?(jwalden+bmo)
Attachment #8545886 - Flags: review?(jwalden+bmo)
Attachment #8545880 - Attachment description: patch → patch 1 - Maybe
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+
(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 :)
Attached patch patch 1 - MaybeSplinter Review
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+
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)
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.

Attachment

General

Created:
Updated:
Size: