(So we can unlock touching the nsStyleAutoArray first's member)

The destructors are needed for bindgen to notice these should not be copiable. I'm not aware of any performance impact they could have, but let me know if there is and we can work around it.

Comment on attachment 8748932 [details] [diff] [review] Proposed patch Review of attachment 8748932 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleStruct.h @@ +467,5 @@ > +template<typename T> > +class nsTArray_Simple { > + T* mBuffer; > +public: > + // This destructor prevents copying in the rust side I would write this as "The existence of a destructor here prevents bindgen from deriving the Clone trait via a simple memory copy". @@ +480,5 @@ > class nsStyleAutoArray > { > public: > + // This destructor prevents copying in the rust side > + ~nsStyleAutoArray() {}; Why do we need this? Shouldn't the annotation propagate upwards from the replaced nsTArray_Simple that we have as a member? @@ +779,5 @@ > + // See the comment in nsTArray_Simple > + static_assert(sizeof(nsTArray_Simple<Layer>) == sizeof(nsTArray<Layer>), > + "nsTArray size mismatch"); > + static_assert(alignof(nsTArray_Simple<Layer>) == alignof(nsTArray<Layer>), > + "nsTArray align mismatch"); Can we make this a macro of the form STATIC_ASSERT_TYPE_LAYOUTS_MATCH(type1, type2)? Then we can use it for all these static assertions, as well as the existing ones we have (which currently lack the alignof assertions).

(In reply to Bobby Holley (busy) from comment #2) > > +public: > > + // This destructor prevents copying in the rust side > > I would write this as "The existence of a destructor here prevents bindgen > from deriving the Clone trait via a simple memory copy". Sounds good. > > + ~nsStyleAutoArray() {}; > > Why do we need this? Shouldn't the annotation propagate upwards from the > replaced nsTArray_Simple that we have as a member? Yes, if this was a normal bindgen type, but actually it doesn't because of how our replaces="" annotation work (in the replacer type instead of in the replaced one). This is a good idea since all annotations aren't spread across the codebase, but has the inconvenience that we can't modify the underlying type directly. I can probably find a easy-ish way of doing this in bindgen though, so I'll take a look at it when I'm less sleepy. > > + static_assert(alignof(nsTArray_Simple<Layer>) == alignof(nsTArray<Layer>), > > + "nsTArray align mismatch"); > > Can we make this a macro of the form STATIC_ASSERT_TYPE_LAYOUTS_MATCH(type1, > type2)? Then we can use it for all these static assertions, as well as the > existing ones we have (which currently lack the alignof assertions). Sounds good too (didn't went with offsets because the offset on the container struct is implied from the alignment, and we're not checking inner fields in nsTArray, since we don't want to access them). I'll figure out a good way to abstract it though. Going to sleep now, but will take a look at it soon :P

Bindgen changes landed, so the second destructor is no longer necessary. I moved all *_Simple types to the bottom so everything stays in the same place.

Sorry for the bugspam. The previous patch was correct, but didn't take in account a last-time renaming I did to be clear about what the macros were doing.

Comment on attachment 8749245 [details] [diff] [review] Patch with more descriptive macro names Review of attachment 8749245 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! r=bholley

Another VS2013 problem I presume? c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3666) : error C2275: 'nsPoint' : illegal use of this type asan expression c:\t1\hg\objdir-sm\dist\include\nsPoint.h(22) : see declaration of 'nsPoint' c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3666) : error C2275: 'nsPoint_Simple' : illegal use of this type as an expression c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3662) : see declaration of 'nsPoint_Simple' c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3666) : error C3861: 'alignof': identifier not found c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3677) : error C2275: 'nsMargin' : illegal use of this type as an expression c:\t1\hg\objdir-sm\dist\include\nsMargin.h(14) : see declaration of 'nsMargin' c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3677) : error C2275: 'nsMargin_Simple' : illegal use of this type as an expression c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3673) : see declaration of 'nsMargin_Simple' c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3677) : error C3861: 'alignof': identifier not found c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3690) : error C2275: 'nsRect' : illegal use of this type as an expression c:\t1\hg\objdir-sm\dist\include\nsRect.h(24) : see declaration of 'nsRect' c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3690) : error C2275: 'nsRect_Simple' : illegal use of this type as an expression c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3686) : see declaration of 'nsRect_Simple' c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3690) : error C3861: 'alignof': identifier not found c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3709) : error C2275: 'nsTArray<T>' : illegal use of this type as an expression with [ T=nsStyleImageLayers::Layer ] c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3709) : error C2275: 'nsTArray_Simple<nsStyleImageLayers::Lay er>' : illegal use of this type as an expression c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3709) : error C3861: 'alignof': identifier not found c:\t1\hg\comm-central\mozilla\layout\style\nsStyleStruct.h(3711) : error C2275: 'nsTArray<T>' : illegal use of this type as an expression with

(In reply to Philip Chee from comment #9) > Another VS2013 problem I presume? According to this msdn page[1]: > Alignment > > The Core Language keywords alignas/alignof from the alignment proposal that was voted into the Working > Paper are implemented in Visual Studio 2015. Visual C++ in Visual Studio 2010 had aligned_storage from > TR1. Visual C++ in Visual Studio 2012 added aligned_union and std::align() to the Standard Library and > significant issues were fixed in Visual C++ in Visual Studio 2013. So yes, it seems that VS2013 never supported `alignof`. We might be able to work around this if absolutely needed (maybe relying on bindgen automatic tests, or maybe just #ifdef ing), but I think it's preferable to keep these static layout tests in tree. [1]: https://msdn.microsoft.com/en-us/library/hh567368.aspx

FWIW other workarounds that arised from grepping the tree that we could use: security/sandbox/chromium/base/compiler_specific.h: > // Return the byte alignment of the given type (available at compile time). Use > // sizeof(type) prior to checking __alignof to workaround Visual C++ bug: > // http://goo.gl/isH0C > // Use like: > // ALIGNOF(int32) // this would be 4 > #if defined(COMPILER_MSVC) > #define ALIGNOF(type) (sizeof(type) - sizeof(type) + __alignof(type)) media/webrtc/trunk/webrtc/overrides/webrtc/base/basictypes.h > #if _MSC_VER < 1700 > #define alignof(t) _alignof(t) > #endif media/libpng/pngpriv.h: > # define png_alignof(type) offsetof(struct{char c; type t;}, t) ^ pretty clever BTW gfx/skia/skia/src/core/SkFindAndPlaceGlyph.h: > #if defined(_MSC_VER) && _MSC_VER < 1900 > #define alignof __alignof > #endif Still I'd prefer to be away of compiler-dependent code.

Emilio, any estimates on when we'll get the fix for this (comment 9)? This is a hard block for some of us, can't actually do any development. If we can't have it quickly, we should back the offending patch out.

(In reply to Milan Sreckovic [:milan] from comment #12) > Emilio, any estimates on when we'll get the fix for this (comment 9)? This > is a hard block for some of us, can't actually do any development. If we > can't have it quickly, we should back the offending patch out. I am not necessarily opposed to asking Emilio to fix this, but I want to be sure that's appropriate. IIUC, VS2013 is not a tier-1 platform at the moment, and we're not running it on CI. Is the situation more nuanced than that somehow? Emilio is a volunteer contributor and does not own a Windows machine. If there are people who are maintaining VS2013 support, doesn't the burden fall on them to implement and land the suggestions in comment 11, and wouldn't it be a lot more efficient to do it that way?

There's an existing polyfill in MFBT [1]. Can we just use that? [1] https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/mfbt/Alignment.h#35

(In reply to Botond Ballo [:botond] from comment #14) > There's an existing polyfill in MFBT [1]. Can we just use that? > > [1] > https://dxr.mozilla.org/mozilla-central/rev/ > 043082cb7bd8490c60815f67fbd1f33323ad7663/mfbt/Alignment.h#35 Almost certainly.

(In reply to Botond Ballo [:botond] from comment #16) > https://dxr.mozilla.org/mozilla-central/rev/ > 043082cb7bd8490c60815f67fbd1f33323ad7663/mfbt/Alignment.h#35 The link I meant to post was: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ccb85625ab4

(In reply to Milan Sreckovic [:milan] from comment #12) > Emilio, any estimates on when we'll get the fix for this (comment 9)? This > is a hard block for some of us, can't actually do any development. If we > can't have it quickly, we should back the offending patch out. Just read this, sorry for the breakage. Seems like comment 16 has an appropriate fix, hopefully is all good to go with that. Feel free to ni me if you need something else I might help with, though as Bobby said, I don't own a suitable windows machine for Gecko development :)

(In reply to Botond Ballo [:botond] from comment #17) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ccb85625ab4 I'm also doing a local build with VS2013 to verify that the workaround works, since VS2013 isn't run in automation.

(In reply to Botond Ballo [:botond] from comment #19) > I'm also doing a local build with VS2013 to verify that the workaround > works, since VS2013 isn't run in automation. Hmm, my VS2013 can't even compile the defaulted move-constructor of Thread::Id in js/src/threading/Thread.h ...

(In reply to Botond Ballo [:botond] from comment #20) > (In reply to Botond Ballo [:botond] from comment #19) > > I'm also doing a local build with VS2013 to verify that the workaround > > works, since VS2013 isn't run in automation. > > Hmm, my VS2013 can't even compile the defaulted move-constructor of > Thread::Id in js/src/threading/Thread.h ... That change was also committed a few days ago. After working around it, I hit yet another place where a commit made within the last few days broke VS2013. Long story short: with VS2013 not running in automation, the VS2013 build is being broken very quickly, and fixing things after the fact isn't really going to scale. I think we need to make a call about either running VS2013 in automation, or dropping support for it.

(In reply to Botond Ballo [:botond] from comment #20) > (In reply to Botond Ballo [:botond] from comment #19) > > I'm also doing a local build with VS2013 to verify that the workaround > > works, since VS2013 isn't run in automation. > > Hmm, my VS2013 can't even compile the defaulted move-constructor of > Thread::Id in js/src/threading/Thread.h ... The fix is in https://bugzilla.mozilla.org/show_bug.cgi?id=956899#c179 For VS2013 there are only three blockers: Bug 1211723, Bug 956899, And this one Bug 1270305

(In reply to Philip Chee from comment #22) > (In reply to Botond Ballo [:botond] from comment #20) > > (In reply to Botond Ballo [:botond] from comment #19) > > > I'm also doing a local build with VS2013 to verify that the workaround > > > works, since VS2013 isn't run in automation. > > > > Hmm, my VS2013 can't even compile the defaulted move-constructor of > > Thread::Id in js/src/threading/Thread.h ... > > The fix is in https://bugzilla.mozilla.org/show_bug.cgi?id=956899#c179 Not really; even with that change: 14:42.82 c:\Users\msreckovic\Repos\mozilla-central\js\src\threading/Thread.h(46) : error C2610: 'js::Thread::Id::Id(js::Thread::Id &&)' : is not a special member function which can be defaulted 14:42.82 c:\Users\msreckovic\Repos\mozilla-central\js\src\threading/Thread.h(48) : error C2610: 'js::Thread::Id &js::Thread::Id::operator =(js::Thread::Id &&)': is not a special member function which can be defaulted 14:42.82 c:/Users/msreckovic/Repos/mozilla-central/js/src/threading/windows/Thread.cpp(55) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called 14:42.82 c:/Users/msreckovic/Repos/mozilla-central/js/src/threading/windows/Thread.cpp(63) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called 14:42.82 c:/Users/msreckovic/Repos/mozilla-central/js/src/threading/windows/Thread.cpp(87) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called 14:42.82 c:/Users/msreckovic/Repos/mozilla-central/js/src/threading/windows/Thread.cpp(96) : error C2264: 'js::Thread::Id::operator =' : error in function definition or declaration; function not called 14:42.82 c:/Users/msreckovic/Repos/mozilla-central/js/src/threading/windows/Thread.cpp(106) : error C2264: 'js::Thread::Id::Id' : error in function definition or declaration; function not called

Comment on attachment 8750477 [details] [diff] [review] Follow-up to un-break VS2013 build. Review of attachment 8750477 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay, this got buried and I missed it. r+ if we still need it.

(In reply to Botond Ballo [:botond] from comment #21) > Long story short: with VS2013 not running in automation, the VS2013 build is > being broken very quickly, and fixing things after the fact isn't really > going to scale. I think we need to make a call about either running VS2013 > in automation, or dropping support for it. The decision is being made in bug 1186064.

(In reply to Bobby Holley (busy) from comment #24) > Sorry about the delay, this got buried and I missed it. r+ if we still need > it. Based on the discussion in bug 1186064, it looks like we're not going to support VS2013 on the 49 branch, so I'm not going to land this.

Closing since MVSC 2013 support is officially gone.