js::Vector should just be a template alias

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: fitzgen, Unassigned)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

2.11 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Attachment #8613717 - Flags: review?(jwalden+bmo)
Note that I didn't remove all the crazy CRTP VectorBase stuff. That's too much for me right now.
Comment on attachment 8613717 [details] [diff] [review]
js::Vector should just be a template alias

Review of attachment 8613717 [details] [diff] [review]:
-----------------------------------------------------------------

This is okay, but please remove mozilla::VectorBase in another patch.  It should largely just be a mass-rename coupled with removing the VectorBase class and removing one or two direct references.  No good reason to put it off.
Attachment #8613717 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8613732 [details] [diff] [review]
Part 2: Remove VectorBase<ThisVector> CRTP craziness

Review of attachment 8613732 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Vector.h
@@ +259,5 @@
> + * T requirements:
> + *  - default and copy constructible, assignable, destructible
> + *  - operations do not throw
> + * N requirements:
> + *  - any value, however, N is clamped to min/max values

s/N/MinInlineCapacity/g

@@ +270,5 @@
>   */
> +template<typename T,
> +         size_t MinInlineCapacity = 0,
> +         class AllocPolicy = MallocAllocPolicy>
> +class Vector : private AllocPolicy

Because our various function arguments take exactly Vector now, people will run into problems if they inherit from Vector and try to use it in place of Vector.

I think you can (in a followup patch) add |final| to this class declaration, but please run it past try first.  r=me for that extra addition.

@@ +1031,3 @@
>  MOZ_ALWAYS_INLINE void
> +Vector<T, N, AP>::internalAppendAll(
> +  const Vector<U, O, BP>& aOther)

This should be a single line.
Attachment #8613732 - Flags: review?(jwalden+bmo) → review+
Seems like a few places define subclasses of js::Vector already -- how should we proceed?

> In file included from /Users/fitzgen/src/mozilla-central/js/src/builtin/RegExp.cpp:7:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/builtin/RegExp.h:10:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/vm/RegExpObject.h:13:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/jscntxt.h:15:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/vm/Runtime.h:31:
> /Users/fitzgen/src/mozilla-central/js/src/gc/GCRuntime.h:538:31: error: base 'Vector' is marked 'final'
> class CallbackVector : public Vector<Callback<F>, 4, SystemAllocPolicy> {};
>                               ^
> /Users/fitzgen/src/mozilla-central/js/src/gc/GCRuntime.h:1257:40: note: in instantiation of template class 'js::gc::CallbackVector<void (*)(JSFreeOp *, JSFinalizeStatus, bool, void *)>' requested here
>     CallbackVector<JSFinalizeCallback> finalizeCallbacks;
>                                        ^
> ../../dist/include/mozilla/Vector.h:274:7: note: 'Vector' declared here
> class Vector final : private AllocPolicy
>       ^      ~~~~~
> In file included from /Users/fitzgen/src/mozilla-central/js/src/builtin/RegExp.cpp:7:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/builtin/RegExp.h:10:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/vm/RegExpObject.h:13:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/jscntxt.h:15:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/vm/Runtime.h:31:
> /Users/fitzgen/src/mozilla-central/js/src/gc/GCRuntime.h:538:31: error: base 'Vector' is marked 'final'
> class CallbackVector : public Vector<Callback<F>, 4, SystemAllocPolicy> {};
>                               ^
> /Users/fitzgen/src/mozilla-central/js/src/gc/GCRuntime.h:1258:43: note: in instantiation of template class 'js::gc::CallbackVector<void (*)(JSRuntime *, void *)>' requested here
>     CallbackVector<JSWeakPointerCallback> updateWeakPointerCallbacks;
>                                           ^
> ../../dist/include/mozilla/Vector.h:274:7: note: 'Vector' declared here
> class Vector final : private AllocPolicy
>       ^      ~~~~~
> In file included from /Users/fitzgen/src/mozilla-central/js/src/builtin/RegExp.cpp:7:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/builtin/RegExp.h:10:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/vm/RegExpObject.h:13:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/jscntxt.h:15:
> In file included from /Users/fitzgen/src/mozilla-central/js/src/vm/Runtime.h:31:
> /Users/fitzgen/src/mozilla-central/js/src/gc/GCRuntime.h:538:31: error: base 'Vector' is marked 'final'
> class CallbackVector : public Vector<Callback<F>, 4, SystemAllocPolicy> {};
>                               ^
> /Users/fitzgen/src/mozilla-central/js/src/gc/GCRuntime.h:1278:35: note: in instantiation of template class 'js::gc::CallbackVector<void (*)(JSTracer *, void *)>' requested here
>     CallbackVector<JSTraceDataOp> blackRootTracers;
>                                   ^
> ../../dist/include/mozilla/Vector.h:274:7: note: 'Vector' declared here
> class Vector final : private AllocPolicy
>       ^      ~~~~~
Flags: needinfo?(jwalden+bmo)
Attachment #8613732 - Attachment is obsolete: true
Attachment #8613832 - Flags: review+
Er wait, only one thing. And it looks like this could be a template alias as well?
Yeah it can be.
Flags: needinfo?(jwalden+bmo)
Attachment #8613833 - Attachment is obsolete: true
Attachment #8613833 - Flags: review?(jwalden+bmo)
Attachment #8614120 - Flags: review?(jwalden+bmo)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #13)
> Created attachment 8614120 [details] [diff] [review]
> Part 3: Make mozilla::Vector a final class

And now with firefox building instead of only SM.
Comment on attachment 8614120 [details] [diff] [review]
Part 3: Make mozilla::Vector a final class

Review of attachment 8614120 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomAnimation.h
@@ +51,3 @@
>      Vector<Task*> result;
>      mDeferredTasks.swap(result);
> +    return mozilla::Move(result);

Nope:

http://stackoverflow.com/questions/28940861/return-a-local-object-rvalue-reference-right-or-wrong

Certainly the return type shouldn't change.  I believe C++ requires that move-construction be used to create the return value, so this should be doing the efficient swap-y thing already.  But if you want to be clearer about that, adding just the mozilla::Move() doesn't hurt.

::: js/src/ctypes/CTypes.h
@@ +26,5 @@
>  *******************************************************************************/
>  
>  // Container class for Vector, using SystemAllocPolicy.
>  template<class T, size_t N = 0>
> +using Array = Vector<T, N, SystemAllocPolicy>;

Let's not lose the static assertion if we can help it.  I think this will do the trick:

template<class T,
         size_t N = 0,
         // Don't use this with JS::Value!  Use JS::AutoValueVector instead.
         typename = typename mozilla::EnableIf<!mozilla::IsSame<T, JS::Value>::value>::Type>
using Array = Vector<T, N, SystemAllocPolicy>;

::: toolkit/components/telemetry/ThreadHangStats.h
@@ +98,5 @@
> +  }
> +  void infallibleAppend(const char* aEntry) { mImpl.infallibleAppend(aEntry); }
> +  bool reserve(size_t aRequest) { return mImpl.reserve(aRequest); }
> +  const char** begin() { return mImpl.begin(); }
> +  const char*const* begin() const { return mImpl.begin(); }

Space before const, surely.

@@ +100,5 @@
> +  bool reserve(size_t aRequest) { return mImpl.reserve(aRequest); }
> +  const char** begin() { return mImpl.begin(); }
> +  const char*const* begin() const { return mImpl.begin(); }
> +  const char** end() { return mImpl.end(); }
> +  const char*const* end() const { return mImpl.end(); }

And.
Attachment #8614120 - Flags: review?(jwalden+bmo) → review+
Attachment #8614120 - Attachment is obsolete: true
Attachment #8613832 - Attachment is obsolete: true
Attachment #8613717 - Attachment is obsolete: true
Don't have time to try and work around windows compiler internal errors.
Assignee: nfitzgerald → nobody
What about if you did

template<class T,
         size_t N = 0
// 1800 is MSVC2013.  Optimistically assume MSVC2015 (1900) is fixed.
// If you're porting to MSVC2015 and this doesn't work, extend the
// condition to encompass that additional version (but *do* keep the
// version-check so we know when MSVC's fixed).
#if !defined(_MSC_VER) || (1800 <= _MSC_VER && _MSC_VER <= 1800))
         // Don't use this with JS::Value!  Use JS::AutoValueVector instead.
         , typename = typename mozilla::EnableIf<!mozilla::IsSame<T, JS::Value>::value>::Type>
#endif
using Array = ...;

to punt the problem for MSVC?  The error pretty clearly tracks back to this, from the looks of it.  And then we'd get the verification at least with compilers that support it, and would canary newer MSVCs as they become available.
Flags: needinfo?(nfitzgerald)
Attachment #8615636 - Attachment is obsolete: true
Attachment #8615638 - Attachment is obsolete: true
Attachment #8615639 - Attachment is obsolete: true
Attachment #8617559 - Attachment is obsolete: true
Finally cleared things up enough here to land.  The MSVC problem that CTypes.cpp and others were hitting, appears to be something related to this:

template<typename T, int N>
class Array
{
};

template<typename T, int N>
using MyArray = Array<T, N>;

template<typename T, int N>
void
Process(MyArray<T, N>& arr)
{
  // error about here or so
}

int main()
{
  MyArray<int, 1> arr;
  Process(arr);
}

Basically, it looks like there are Problems when you use a template alias in a template function signature (possibly where the template alias is used with a template parameter type, i.e. MyArray<T, N>).  I can't reproduce it locally with MSVC 2013 Update 4; tinderboxen are using Update 3.  But I managed to get green try-builds  if I replaced MyArray in the signature with Array (in our code, js::Vector with mozilla::Vector), so I just did that everywhere.

With that, I packaged all these changes into one patch and landed it.
https://hg.mozilla.org/mozilla-central/rev/30044858f40c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
It seems to me that the change made in this patch to the signature of AsyncPanZoomAnimation::TakeDeferredTask() is unnecessary. If no one objects, I'm going to revert it.

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebee09101131
Attachment #8698240 - Flags: review?(bugmail.mozilla)
Hmm.  Why was that there, indeed.  Fine by me -- although making the method just |return mozilla::Move(mDeferredTasks);| would be even simpler than this swap-and-return routine, and possibly slightly more efficient if the compiler doesn't recognize the local can be elided, with the swap occurring into the return value location on the stack.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #28)
> Hmm.  Why was that there, indeed.  Fine by me -- although making the method
> just |return mozilla::Move(mDeferredTasks);| would be even simpler than this
> swap-and-return routine, and possibly slightly more efficient if the
> compiler doesn't recognize the local can be elided, with the swap occurring
> into the return value location on the stack.

Yep, good idea.
Attachment #8698244 - Flags: review?(jwalden+bmo)
Attachment #8698240 - Attachment is obsolete: true
Attachment #8698240 - Flags: review?(bugmail.mozilla)
Attachment #8698244 - Flags: review?(jwalden+bmo) → review+
Depends on: 1289987
You need to log in before you can comment on or make changes to this bug.