Closed
Bug 1170325
Opened 9 years ago
Closed 8 years ago
js::Vector should just be a template alias
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: fitzgen, Unassigned)
References
Details
Attachments
(1 file, 10 obsolete files)
2.11 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
I believe we have the necessary compiler support now. https://mxr.mozilla.org/mozilla-central/source/js/public/Vector.h?rev=02f2f4c75007#22
Reporter | ||
Comment 1•9 years ago
|
||
Attachment #8613717 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 2•9 years ago
|
||
Note that I didn't remove all the crazy CRTP VectorBase stuff. That's too much for me right now.
Comment 3•9 years ago
|
||
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+
Reporter | ||
Comment 4•9 years ago
|
||
Attachment #8613732 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93f0178353c0
Comment 6•9 years ago
|
||
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+
Reporter | ||
Comment 7•9 years ago
|
||
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)
Reporter | ||
Comment 8•9 years ago
|
||
Attachment #8613732 -
Attachment is obsolete: true
Attachment #8613832 -
Flags: review+
Reporter | ||
Comment 9•9 years ago
|
||
Er wait, only one thing. And it looks like this could be a template alias as well?
Reporter | ||
Comment 10•9 years ago
|
||
Attachment #8613833 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 12•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7668e1cc56d8
Reporter | ||
Comment 13•9 years ago
|
||
Attachment #8613833 -
Attachment is obsolete: true
Attachment #8613833 -
Flags: review?(jwalden+bmo)
Attachment #8614120 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 14•9 years ago
|
||
(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 15•9 years ago
|
||
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+
Reporter | ||
Comment 16•9 years ago
|
||
Attachment #8615636 -
Flags: review+
Reporter | ||
Comment 17•9 years ago
|
||
Attachment #8615638 -
Flags: review+
Reporter | ||
Comment 18•9 years ago
|
||
Attachment #8615639 -
Flags: review+
Reporter | ||
Comment 19•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af15ffc6d8c4
Reporter | ||
Updated•9 years ago
|
Attachment #8614120 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8613832 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8613717 -
Attachment is obsolete: true
Reporter | ||
Comment 20•9 years ago
|
||
Don't have time to try and work around windows compiler internal errors.
Assignee: nfitzgerald → nobody
Comment 21•9 years ago
|
||
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)
Reporter | ||
Comment 22•9 years ago
|
||
Attachment #8617559 -
Flags: review+
Reporter | ||
Comment 23•9 years ago
|
||
Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a11322174eb
Flags: needinfo?(nfitzgerald)
Reporter | ||
Updated•9 years ago
|
Attachment #8615636 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8615638 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8615639 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8617559 -
Attachment is obsolete: true
Comment 25•8 years ago
|
||
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.
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30044858f40c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 27•8 years ago
|
||
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)
Comment 28•8 years ago
|
||
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.
Comment 29•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8698240 -
Attachment is obsolete: true
Attachment #8698240 -
Flags: review?(bugmail.mozilla)
Updated•8 years ago
|
Attachment #8698244 -
Flags: review?(jwalden+bmo) → review+
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e11cb4667aa0
You need to log in
before you can comment on or make changes to this bug.
Description
•