[C++] Got rid of memset's in constructors by bakinovsky-m · Pull Request #5938 · google/flatbuffers

bakinovsky-m · 2020-05-29T13:55:51Z

There was a std::memset's in constructors of structs. GCC 10.1 failed to compile arrays_test_generated.h because of those memsets on array of non-trivial type NestedStruct

In this new code:

default constructor have no memset's now - it zero- or default-initialize all it's members
so does parametrized constructor
array of non-POD types are initializing by calling default-constructor for each of its object

now POD-typed arrays are zero-initialized and class-typed arrays are default-initialized

googlebot · 2020-05-29T13:55:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

tests/arrays_test_generated.h

aardappel · 2020-05-29T16:48:23Z

tests/arrays_test_generated.h

+  {
+    (void)padding0__;
+    (void)padding1__;
+    (void)padding2__;


Do we still need these void casts?

unfortunately, yes
otherwise, compiler (clang 10.0) says something like

In file included from flatbuffers/tests/test.cpp:39: flatbuffers/build/tests/arrays_test_generated.h:71:10: error: private field 'padding0__' is not used [-Werror,-Wunused-private-field] int8_t padding0__; int32_t padding1__;

Can we make a FLATBUFFERS_UNUSED() macro or something to better give intent to why we are using (void)?

tests/arrays_test_generated.h

tests/test.cpp

bakinovsky-m · 2020-05-29T19:39:47Z

@googlebot I signed it!

googlebot · 2020-05-29T19:39:56Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

dbaileychess · 2020-05-29T20:45:32Z

src/idl_gen_cpp.cpp

+        // for each object in array it:
+        // * sets it as zeros for POD types (integral, floating point, etc)
+        // * calls default constructor for classes/structs
+        init_list += field_name + "()";


I would move init_list += field_name out of the conditional, and just have the "()" : "(0)" part in a ternary conditional.

ok, i'll move field_name out
but it seems like if-else construction allows this case-specific comment to be more readable than in ternary conditional, what do you think?

init_list += (IsStruct(field->value.type) || IsArray(field->value.type)) // this is either default initialization of struct // or // implicit initialization of array // for each object in array it: // * sets it as zeros for POD types (integral, floating point, etc) // * calls default constructor for classes/structs ? "()" : "(0)";

versus

if (IsStruct(field->value.type) || IsArray(field->value.type)) { // this is either default initialization of struct // or // implicit initialization of array // for each object in array it: // * sets it as zeros for POD types (integral, floating point, etc) // * calls default constructor for classes/structs init_list += "()"; } else { init_list += "(0)"; }

Either is fine with me, with a slight preference for the ternary.

src/idl_gen_cpp.cpp

dbaileychess · 2020-05-29T20:51:10Z

tests/arrays_test_generated.h

+  {
+    (void)padding0__;
+    (void)padding1__;
+    (void)padding2__;


Can we make a FLATBUFFERS_UNUSED() macro or something to better give intent to why we are using (void)?

bakinovsky-m · 2020-05-30T10:18:36Z

Can anybody help me with this Visual Studio build errors?

It looks like Visual Studio just wants to tell, that behaviour in this place changed from incorrect to correct (link). But due to warning-as-error setting build is failing. Maybe I can somehow suppress this particular warning?

https://ci.appveyor.com/project/google-admin/flatbuffers/builds/33204251/job/d535rfh8x87kyv1n

c:\projects\flatbuffers\tests\arrays_test_generated.h(84): error C2220: warning treated as error - no 'object' file generated [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(84): warning C4351: new behavior: elements of array 'MyGame::Example::NestedStruct::a_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(84): warning C4351: new behavior: elements of array 'MyGame::Example::NestedStruct::c_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(84): warning C4351: new behavior: elements of array 'MyGame::Example::NestedStruct::d_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(94): warning C4351: new behavior: elements of array 'MyGame::Example::NestedStruct::a_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(94): warning C4351: new behavior: elements of array 'MyGame::Example::NestedStruct::c_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(94): warning C4351: new behavior: elements of array 'MyGame::Example::NestedStruct::d_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(161): warning C4351: new behavior: elements of array 'MyGame::Example::ArrayStruct::b_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(161): warning C4351: new behavior: elements of array 'MyGame::Example::ArrayStruct::f_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(177): warning C4351: new behavior: elements of array 'MyGame::Example::ArrayStruct::b_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
c:\projects\flatbuffers\tests\arrays_test_generated.h(177): warning C4351: new behavior: elements of array 'MyGame::Example::ArrayStruct::f_' will be default initialized [C:\projects\flatbuffers\flattests.vcxproj]
C:\projects\flatbuffers\tests\test.cpp(3232): error C2061: syntax error : identifier 'non_zero_memory' [C:\projects\flatbuffers\flattests.vcxproj]

aardappel · 2020-06-01T16:01:41Z

@bakinovsky-m yes, sounds like we can disable that warning: https://docs.microsoft.com/en-us/previous-versions/1ywe7hcy(v=vs.140)?redirectedfrom=MSDN

Sadly, that would mean this at the start of a generated code file:

#if defined(_MSC_VER)
  #pragma warning(push)
  #pragma warning(disable: 4351) // C4351: new behavior: elements of array 'array' will be default initialized
#endif

And this at the end:

#if defined(_MSC_VER)
  #pragma warning(pop)
#endif

Though frankly, I'd also be ok with just sticking this in base.h, without the push/pop, since this is really weird warning that apparently is only there for people "upgrading" their code, and is not a warning other compilers give. @dbaileychess ? @vglavnyy ?

As for non_zero_memory.. maybe needs an explicit & in front? Or failing that, &non_zero_memory[0] or something :(

vglavnyy · 2020-06-01T16:29:12Z

Another possible solution:

struct Foo {
	int arr[10];

#ifdef _MSC_VER
	__pragma(warning(disable : 4351));  // C4351: new behavior: elements of array 'array' will be default initialized
#endif
	Foo() : arr()
	{}
};

aardappel · 2020-06-01T16:36:28Z

@vglavnyy yeah, but having that in each and every struct is a bit cluttered to read..

aardappel · 2020-06-01T16:41:51Z

Also, this is a warning given by VS2013, and the description says that this constructor didn't always cause zero-init in previous versions.. but we also still compile for VS2010! So in theory, it could not work there, and not give us zero-init. I guess our unit-test will catch that if that is case?

Like I said, I'd be fine to do it globally in base.h. The worst than can happen is that someone's code that sits in the same .cpp as FlatBuffers is included contains a datatype that has an explicit array constructor and relied on a very old C++ standard and non-initialization for.. performance? I find that exceedingly unlikely.

aardappel · 2020-06-01T19:44:44Z

The CI fail you just got is crtdbg.h clashing with placement new (see top of base.h), if we want to keep using it, you may need to #undef new before it, and #define new DEBUG_NEW after (if _WIN32 and !NDEBUG). Or maybe there's a way to do this test without placement new??

aardappel · 2020-06-02T00:58:45Z

Ok, awesome, thanks!

* [C++] removed array's memsets from struct parametrized constructor now POD-typed arrays are zero-initialized and class-typed arrays are default-initialized * [C++] memset -> zero/default initialization in default constructor * [C++] Struct-type and array default initialization * [C++] Newly generated tests * [C++] forgotten test * [C++] curly brace by code style * [C++] test if memory is 0's after placement new * [C++] memory leak fix * [C++] simplifying and non-dynamic memory in test * [C++] code cleanup * [C++] disable old-compiler warning * [C++] windows build fix (try) * [C++] debug-new win build fix

bakinovsky-m added 4 commits May 29, 2020 16:02

[C++] removed array's memsets from struct parametrized constructor

70744b6

now POD-typed arrays are zero-initialized and class-typed arrays are default-initialized

[C++] memset -> zero/default initialization in default constructor

e6607a5

[C++] Struct-type and array default initialization

7d86517

[C++] Newly generated tests

b9dc0c6

bakinovsky-m changed the title ~~[C++] Got rid of memset's in constuctors~~ [C++] Got rid of memset's in constructors May 29, 2020

[C++] forgotten test

d23de8e

aardappel reviewed May 29, 2020

View reviewed changes

[C++] curly brace by code style

63e3723

bakinovsky-m force-pushed the memset-ctrs branch from 7eda486 to 63e3723 Compare May 29, 2020 17:25

bakinovsky-m added 2 commits May 29, 2020 21:59

[C++] test if memory is 0's after placement new

88cdd6f

[C++] memory leak fix

228a157

aardappel reviewed May 29, 2020

View reviewed changes

tests/test.cpp Outdated Show resolved Hide resolved

tests/test.cpp Outdated Show resolved Hide resolved

[C++] simplifying and non-dynamic memory in test

90d7534

dbaileychess reviewed May 29, 2020

View reviewed changes

[C++] code cleanup

e51f2ad

bakinovsky-m added 2 commits June 1, 2020 20:13

[C++] disable old-compiler warning

491c778

[C++] windows build fix (try)

407fd19

[C++] debug-new win build fix

bc682ad

aardappel merged commit 988164f into google:master Jun 2, 2020

wdconinc mentioned this pull request May 5, 2022

flatbufffers (regression): patches are in newly added v2.0.0 already spack/spack#30492

Merged

mogemimi mentioned this pull request Jun 10, 2023

Fix warning C4619 regarding C4351 in Visual Studio 2015 or later #7999

Closed

Lukasdoe mentioned this pull request Oct 16, 2023

Remove msvc pragma warning disable C4351 as it is undocumented #8124

Merged

Conversation

bakinovsky-m commented May 29, 2020

Uh oh!

googlebot commented May 29, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Uh oh!

aardappel May 29, 2020

Choose a reason for hiding this comment

Uh oh!

bakinovsky-m May 29, 2020

Choose a reason for hiding this comment

Uh oh!

dbaileychess May 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bakinovsky-m commented May 29, 2020

Uh oh!

googlebot commented May 29, 2020

Uh oh!

dbaileychess May 29, 2020

Choose a reason for hiding this comment

Uh oh!

bakinovsky-m May 30, 2020

Choose a reason for hiding this comment

Uh oh!

dbaileychess Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbaileychess May 29, 2020

Choose a reason for hiding this comment

Uh oh!

bakinovsky-m commented May 30, 2020

Uh oh!

aardappel commented Jun 1, 2020

Uh oh!

vglavnyy commented Jun 1, 2020

Uh oh!

aardappel commented Jun 1, 2020

Uh oh!

aardappel commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardappel commented Jun 1, 2020

Uh oh!

aardappel commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aardappel commented Jun 1, 2020 •

edited

Loading