Fix MSVC build by svenk177 · Pull Request #5508 · google/flatbuffers

svenk177 · 2019-09-05T11:09:07Z

This PR should (hopefully) fix the currently failing build on Visual Studio.
This got overlooked in my last PR (#5491), as the build job was never actually executed for some reason.

aardappel

Thanks for the fix.. though if all this does is allow structs to be passed to these functions, it is still incorrect. Please see discussion at the end of #5313
@vglavnyy

aardappel · 2019-09-05T18:05:20Z

include/flatbuffers/base.h

 #endif

-template<typename T> T EndianSwap(T t) {
+template<typename T> T EndianSwap(const T &t) {


This change worries me.. this function is meant for scalars only, and if any compiler anywhere compiles this more slowly than just T, we can't have it.

With just T MSVC doesn't seem to propagate the __declspec(align()) attribute if I understand the issue correctly. Making it a const-ref bypasses this.

aardappel · 2019-09-05T18:05:49Z

include/flatbuffers/base.h

  } else if (sizeof(T) == 2) {
-    union { T t; uint16_t i; } u;
-    u.t = t;
+    union { T t; uint16_t i; } u = { t };


Interesting.. making the assignment an initialization makes the error go away?

Without this change MSVC reports: 'flatbuffers::EndianSwap::<unnamed-type-u>::<unnamed-type-u>(void)': attempting to reference a deleted function

Note EndianSwap is only used on big endian machines by EndianScalar, so I commented the #if FLATBUFFERS_LITTLEENDIAN to test it.

Ok, but can we then make EndianScalar just T again? EndianSwap is fine since it is only ever used on big endian, but EndianScalar should be as obvious a no-op as possible for more naive compilers.

Same with WriteScalar

Unfortunately the const-ref in WriteScalar is required otherwise C2719 't': formal parameter with requested alignment of 8 won't be aligned will persist, as the if-path will be evaluated even if it is not used.

Agree with @vglavnyy, lets try the original plan then. EndianScalar (and to some extent WriteScalar) are such core functions in FlatBuffers, called absolutely everywhere, we can't afford the risk that this will regress performance on some compilers.

There is another issue. The Get function uses ReadScalar via IndirectHelper. This of course will break on big endian machines. I thought about intorducing a GetStruct function, but then iterators and operator[] will be unusable in the current form.

Why would that not work? IndirectHelper's 3rd case is for struct. It's the same code as for Vector which should work :)

MSVC is probably weak when it comes to template specialization :(
I'm getting:

error C2719: 't': formal parameter with requested alignment of 8 won't be aligned
note: see reference to function template instantiation 'T flatbuffers::EndianScalar<T>(T)' being compiled
note: while compiling class template member function 'MyGame::Example::NestedStruct flatbuffers::IndirectHelper<T>::Read(const uint8_t *,flatbuffers::uoffset_t)'
note: see reference to function template instantiation 'MyGame::Example::NestedStruct flatbuffers::IndirectHelper<T>::Read(const uint8_t *,flatbuffers::uoffset_t)' being compiled`
note: see reference to class template instantiation 'flatbuffers::IndirectHelper<T>' being compiled
note: see reference to class template instantiation 'flatbuffers::Array<MyGame::Example::NestedStruct,2>' being compiled

Probably, the best solution will be split Array class into two independent classes:
They have different iterators and Read/Write methods.

Array<T> - current implementation.

ArrayOfStruct<S> - a new implementation.

@aardappel ?

include/flatbuffers/base.h

svenk177 · 2019-09-06T06:24:02Z

I was probably too quick by going the cure the symptom not the disease route!
I like the proposal by @vglavnyy in #5313 allowing structs to be passed to Mutate.
The other changes are still required though, as MSVC will evaluate the if path anyway and fail to compile it.

EDIT: Seems like VS2013 does not like the solution. Probably we have to use GetMutablePointer or introduce MutateStruct. Alternatively we can inhibit the warning.

include/flatbuffers/flatbuffers.h

vglavnyy · 2019-09-15T16:36:49Z

@aardappel, @svenk177
I have tested another solution with Array specializations (all CI passed).
vglavnyy@857b97a

I can make PR if this solution is fully acceptable.
Or @svenk177 can reuse it in this PR.

svenk177 · 2019-09-16T06:04:56Z

@vglavnyy This looks great. I thought about a similar solution, but was worried that the code will explode. Now that I look at it, it doesn't seem so bad :)
@aardappel I think we should use this 👍

vglavnyy · 2019-09-16T06:56:22Z

It is possible to fold these three Array into two or one classes with the help of std::conditional and std::enable_if but Android build doesn't have full-featured STL <traits>.

aardappel · 2019-09-16T18:10:53Z

Yeah, it's a shame there's some duplication there, but if this is more solid, let's use @vglavnyy's implementation.

vglavnyy · 2019-09-17T02:31:36Z

@aardappel
I wrote a new version without code duplication.
But I'm not sure that this templated version is better than the duplication.
The error messages from this SFINAE code will be intimidating.
Make PR with the first variant?

aardappel · 2019-09-19T17:03:00Z

@vglavnyy I'd be fine with non-duplicated one as well, if it passes CI. But, in the end, your call, if the duplicated version is easier to use, that's ok too.

…e#5508) - Tag dispatching is used for implicit specialization - Array<scalar> and Array<struct> have different iterators and accessors - Array<scalar> and Array<struct> have different Mutate() methods

* Draft with Array specialization (#5508) * Array specialization + SFINAE to fold copy-paste (#5508) * Add implicit specialization of Array<scalar> and Array<struct> (#5508) - Tag dispatching is used for implicit specialization - Array<scalar> and Array<struct> have different iterators and accessors - Array<scalar> and Array<struct> have different Mutate() methods * Add implicit specialization of Array<scalar> and Array<struct> (#5508) - Tag dispatching is used for implicit specialization - Array<scalar> and Array<struct> have different iterators and accessors - Array<scalar> and Array<struct> have different Mutate() methods

…5526) * Draft with Array specialization (google#5508) * Array specialization + SFINAE to fold copy-paste (google#5508) * Add implicit specialization of Array<scalar> and Array<struct> (google#5508) - Tag dispatching is used for implicit specialization - Array<scalar> and Array<struct> have different iterators and accessors - Array<scalar> and Array<struct> have different Mutate() methods * Add implicit specialization of Array<scalar> and Array<struct> (google#5508) - Tag dispatching is used for implicit specialization - Array<scalar> and Array<struct> have different iterators and accessors - Array<scalar> and Array<struct> have different Mutate() methods

svenk177 force-pushed the fix-msvc-build branch from 4a852ce to 5521d3e Compare September 5, 2019 11:10

Fix MSVC build

2845ab8

svenk177 force-pushed the fix-msvc-build branch from 5521d3e to 2845ab8 Compare September 5, 2019 11:11

aardappel reviewed Sep 5, 2019

View reviewed changes

Allow structs in Mutate

f17467a

Inhibit warning

8c55db6

vglavnyy reviewed Sep 6, 2019

View reviewed changes

include/flatbuffers/flatbuffers.h Outdated Show resolved Hide resolved

Use IsConstTrue

9ef55e8

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 15, 2019

Proposal with Array specialization google#5508 to test CI

464e244

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 15, 2019

Draft with Array specialization (google#5508)

5776e5c

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 15, 2019

Draft with Array specialization (google#5508)

857b97a

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 17, 2019

Array specialization + SFINAE to fold copy-paste (google#5508)

a5740fa

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 20, 2019

Replace std::enable_if by tag dispatching (google#5508)

08847d9

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 20, 2019

Replace std::enable_if by tag dispatching - android fix (google#5508)

1757efb

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Sep 20, 2019

Replace std::enable_if by tag dispatching - android fix-2 (google#5508)

1a7a10c

vglavnyy mentioned this pull request Sep 20, 2019

Fix Mutate() methods of Array<scalar/struct> (override 5508) #5526

Merged

svenk177 closed this Sep 23, 2019

Conversation

svenk177 commented Sep 5, 2019

Uh oh!

aardappel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

svenk177 commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vglavnyy commented Sep 15, 2019

Uh oh!

svenk177 commented Sep 16, 2019

Uh oh!

vglavnyy commented Sep 16, 2019

Uh oh!

aardappel commented Sep 16, 2019

Uh oh!

vglavnyy commented Sep 17, 2019

Uh oh!

aardappel commented Sep 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

svenk177 commented Sep 6, 2019 •

edited

Loading