Add flatc '--cpp_std' switch #5656

vglavnyy · 2019-12-08T09:39:20Z

Add flatc '--cpp_std' switch and sandbox for C++17 code generator:

Added 'flac --cpp_std legacy' for compatibility with old compilers (VS2010);
Added experimental switch 'flac --cpp_std c++17' for future development;
Added C++17 sandbox test_cpp17.cpp;
C++ code generator generates enums with explicit underlying type to avoid problems with the forward and backward schema compatibility;
Adjusted CMakeLists.txt, CI and generate code scripts to support of introduced '--cpp_std';

This PR will change all already generated C++ code.
By default, generated enums will be declared with an explicit underlying type matched with a schema. For backward compatibility, a flatc switch --cpp_std legacy was added.
For future development an experimental switch --cpp_std c++17 was added.
Indirectly related issues: #5585, #5285, #5626, #5611.

aardappel

Nice, this will allow us to start generating more modern code.

docs/source/Compiler.md

src/flatc.cpp

src/idl_gen_cpp.cpp

aardappel · 2019-12-09T18:19:49Z

include/flatbuffers/flatc.h

+  // There is no way to notify about an error from a code-generator
+  // except `assert` call.
+  // TODO: re-design interface to code generators.
+  static void Warn(const std::string &warn);


If these are static, why aren't they simple extern functions?

They should be extern functions, I will fix it.

The compilers have a non-extensible interface:

flatbuffers/include/flatbuffers/flatc.h

Lines 34 to 40 in eddebec

struct Generator {

typedef bool (*GenerateFn)(const flatbuffers::Parser &parser,

const std::string &path,

const std::string &file_name);

typedef std::string (*MakeRuleFn)(const flatbuffers::Parser &parser,

const std::string &path,

const std::string &file_name);

I've tried to replace the pack of args (parser, path, file_name) by reference to struct or class object with additional logging methods but refused due to too big changes in the code.

src/idl_gen_cpp.cpp

aardappel · 2019-12-09T18:21:37Z

src/idl_parser.cpp

@@ -627,12 +627,6 @@ CheckedError Parser::ParseType(Type &type) {
            "length of fixed-length array must be positive and fit to "
            "uint16_t type");
      }
-      // Check if enum arrays are used in C++ without specifying --scoped-enums


Why is it better to do this in the code generator?

When other langs will support arrays this check will block flatc.

The g_only_fixed_enums flag in idl_gen_cpp is a more general flag. It can be set even if --scoped-enums is inactive (C++11 unscoped enum with explicit underlying type).

aardappel · 2019-12-18T19:18:32Z

There are CI errors: "src\idl_gen_cpp.cpp(2992): warning C4482: nonstandard extension used: enum 'flatbuffers::cpp::CppStandard' used in qualified name [C:\projects\flatbuffers\flatc.vcxproj]"

It may well be that C++11 cannot be the default for the checked-in generated code, because of our VS2010 and Android CI.

- Added 'flac --cpp_std legacy' for compatibility with old compilers (VS2010); - Added experimental switch 'flac --cpp_std c++17' for future development; - Added C++17 sandbox test_cpp17.cpp; - C++ code generator generates enums with explicit underlying type to avoid problems with the forward and backward schema compatibility; - Adjusted CMakeLists.txt, CI and generate code scripts to support of introduced '--cpp_std';

vglavnyy · 2019-12-19T14:42:10Z

It may well be that C++11 cannot be the default for the checked-in generated code,

Fixed. PR was rebased to the master for compatibility with the latest changes in cpp-gen.

If code is ok, we can merge it into the master after the next relsease.
This will give time to add --cpp-std c++0x flag into projects with old compilers.

Maybe add more information about C++ compatibility options into support.md ?
Platform: Linux | GCC282
GCC282 should be GCC-4.82?
Problem: there are no links to this file in the documentation.

aardappel · 2019-12-19T16:21:29Z

Well, we can merge this now with C++0x as default, and then simply swap default after release?

Sadly I guess there is no automatic way to identify those people that will need --cpp-std c++0x, they will simply run into build errors.

vglavnyy · 2019-12-19T19:25:36Z

Really, this will be better.

Travis failed in a strange way. On my repo with the same commit, Travis passed.
I take the time to check this error.

aardappel · 2019-12-19T23:00:35Z

The error appears to be:
TEST FAILED: /Users/travis/build/google/flatbuffers/grpc/tests/message_builder_test.cpp:257, 'true' != 'release_n_verify(mb, m1_name, m1_color)' in void builder_move_ctor_conversion_before_finish_test()

Let me know when you think this is ok to merge.

- Fix dependency declaration of grpctest target

vglavnyy · 2019-12-22T16:33:29Z

@aardappel GRPC-CI fixed, could you review the latest commit?
It was lucky to catch that error before merging.

Conditions:

Target grpctest had declared dependency monster_generated.h instead of monster_test_generated.h.
I forgot to set --cpp_std C++11 for tests in CMakeLists.txt when changed the default from C++11 to C++0x.
Module test_builder.h exported external variables (m1_color, m2_color) instead of functions.
Use parallel build (cmake --build -jN).

Sequence-1 is invalid but could pass CI:

Checkout and configure build. All generated files are files from the repo with --cpp_std C++11.
Build of grpctest before flatc target finished and before all generated files updated.
All observed declarations of MyGame::Example::Color enum were C++11 enum with uint8_t.
Test passed.

Sequence-2, CI failed with error:

Checkout and configure build. All generated files are files from the repo with --cpp_std C++11.
Start compilation of grpctest/test_builder.cpp with monster_test_generated.h before flatc finished. This file will reserve uint_8 for m1_color variable (C3).
Build flatc target and re-generate all headers. Due to C2, all files now C++0x with old-style MyGame::Example::Color enum (underlying type is int).
Start compilation of grpctest.cpp and message_builder_test.cpp with updated monster_test_generated.h (C++0x due to C2).
On test run, message_builder_test.cpp expects that m1_color is int instead of uint8_t and it can read garbage like 513 instead of 1.

Log with failed CI
In this log grpctest was built before monster_test_generated.h was updated:
https://travis-ci.org/google/flatbuffers/jobs/627346280#L2444-L2468.

no-more-secrets · 2019-12-22T19:31:45Z

Thanks for this; when this is merged then I will submit a PR to add in the new table Create methods that we have discussed (I already sent PR 5674, which you can look at as a preview of what I will send).

aardappel · 2019-12-23T17:20:16Z

Thanks for finding what was happening with the CI. So are we able to create explicit dependencies in CMake such that this will always execute in the right order, such that this problem doesn't happen anymore? Or do we need to turn off parallel build?

This PR still seems to default to C++11, I thought we were going to merge it with C++0x default, then change to C++11 after the next release?

…alue (C++0x)

vglavnyy · 2019-12-23T19:20:26Z

I think that all the problems with the dependencies were fixed. It will be better if the parallel compilation remains enabled. It speedups the CI and surfaces possible problems with test code.
I wrote todo about custom_target which would call generate_code.sh. This target should depend on flatc. In turn, other targets will depend on this custom target using target_link_libraries().
This solution would stop further problems with dependencies.

The flatc uses C++0x by default but all tests are generated with --cpp_std C++11 expect for Appveyor-CI MSV2010. This CI runs CMake with flag FLATBUFFERS_BUILD_LEGACY.

I have reverted generate_code.sh scripts to the state as they were two commits earlier.
Now, --cpp_std value for tests is controlled by flatc default settings.

P.S. Failure in Appveyor is unrelated. It failed in Rust tests.

aardappel · 2019-12-23T19:26:50Z

Ok, I will merge this then, thanks!

no-more-secrets · 2019-12-23T19:30:28Z

@vglavnyy Given these changes, could you explain what steps I should take to make a PR with C++17 changes to the C++ generator? Thanks

vglavnyy · 2019-12-24T03:21:27Z

@dpacbach there are no restrictions or right ways, you can do as you wish.

The test_cpp17.cpp has two top-level namespaces: cpp17 and cpp11. The cpp11 namespace added for comparisons if need to check the result with the reference implementation using TEST_EQ as test.cpp does.
A proposed code could be placed inside cpp17 namespace as if it was generated by the generator. After researching, the code can be moved into the generator.
If code looks well you can skip this stage and add it into the generator.
This two-stage procedure without modifying the cpp-generator code at the first stage could simplify researching.

* Add flatc '--cpp_std' switch and sandbox for C++17 code generator - Added 'flac --cpp_std legacy' for compatibility with old compilers (VS2010); - Added experimental switch 'flac --cpp_std c++17' for future development; - Added C++17 sandbox test_cpp17.cpp; - C++ code generator generates enums with explicit underlying type to avoid problems with the forward and backward schema compatibility; - Adjusted CMakeLists.txt, CI and generate code scripts to support of introduced '--cpp_std'; * Fix --cpp_std values: c++0x, c++11, c++17 * Add 'cpp::CppStandard' enum * Add testing engine into test_cpp17 * Rebase to upstream/master * Set default '--cpp-std C++0x' * Fix code generation (--cpp_std C++11) in CMakeLists.txt - Fix dependency declaration of grpctest target * Revert --cpp-std for the tests from explicit C++11 to flatc default value (C++0x)

aardappel reviewed Dec 9, 2019

View reviewed changes

vglavnyy added 5 commits December 19, 2019 07:44

Fix --cpp_std values: c++0x, c++11, c++17

c17e1ec

Add 'cpp::CppStandard' enum

74454e3

Add testing engine into test_cpp17

2d0b123

Rebase to upstream/master

dfc4c9b

vglavnyy force-pushed the add_modern_cpp_test branch from 472c255 to dfc4c9b Compare December 19, 2019 00:47

Set default '--cpp-std C++0x'

0214897

vglavnyy mentioned this pull request Dec 22, 2019

[C++] Add --cpp-extra flatc flag and add new generic table creation methods. #5674

Closed

vglavnyy force-pushed the add_modern_cpp_test branch 2 times, most recently from 7ec95ef to 9132786 Compare December 22, 2019 14:55

Fix code generation (--cpp_std C++11) in CMakeLists.txt

d8a3f10

- Fix dependency declaration of grpctest target

vglavnyy force-pushed the add_modern_cpp_test branch from 9132786 to d8a3f10 Compare December 22, 2019 16:20

Revert --cpp-std for the tests from explicit C++11 to flatc default v…

75fbd4d

…alue (C++0x)

aardappel force-pushed the master branch from 6033222 to 3e8f15d Compare December 23, 2019 19:50

aardappel merged commit 44bf719 into google:master Dec 23, 2019

vglavnyy mentioned this pull request Nov 17, 2020

Fuzzing FlatBuffers Enums (in C++) #6267

Closed

vglavnyy mentioned this pull request Dec 5, 2020

[C++] Switch flatc to --cpp-std c++11 C++ code generator #6306

Merged

vglavnyy deleted the add_modern_cpp_test branch June 19, 2021 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flatc '--cpp_std' switch #5656

Add flatc '--cpp_std' switch #5656

vglavnyy commented Dec 8, 2019

aardappel left a comment

aardappel Dec 9, 2019

vglavnyy Dec 16, 2019

aardappel Dec 9, 2019

vglavnyy Dec 16, 2019

aardappel commented Dec 18, 2019

vglavnyy commented Dec 19, 2019

aardappel commented Dec 19, 2019

vglavnyy commented Dec 19, 2019

aardappel commented Dec 19, 2019

vglavnyy commented Dec 22, 2019 •

edited

Loading

no-more-secrets commented Dec 22, 2019

aardappel commented Dec 23, 2019

vglavnyy commented Dec 23, 2019 •

edited

Loading

aardappel commented Dec 23, 2019 •

edited

Loading

no-more-secrets commented Dec 23, 2019

vglavnyy commented Dec 24, 2019

	struct Generator {
	typedef bool (*GenerateFn)(const flatbuffers::Parser &parser,
	const std::string &path,
	const std::string &file_name);
	typedef std::string (*MakeRuleFn)(const flatbuffers::Parser &parser,
	const std::string &path,
	const std::string &file_name);

Add flatc '--cpp_std' switch #5656

Add flatc '--cpp_std' switch #5656

Conversation

vglavnyy commented Dec 8, 2019

aardappel left a comment

Choose a reason for hiding this comment

aardappel Dec 9, 2019

Choose a reason for hiding this comment

vglavnyy Dec 16, 2019

Choose a reason for hiding this comment

aardappel Dec 9, 2019

Choose a reason for hiding this comment

vglavnyy Dec 16, 2019

Choose a reason for hiding this comment

aardappel commented Dec 18, 2019

vglavnyy commented Dec 19, 2019

aardappel commented Dec 19, 2019

vglavnyy commented Dec 19, 2019

aardappel commented Dec 19, 2019

vglavnyy commented Dec 22, 2019 • edited Loading

no-more-secrets commented Dec 22, 2019

aardappel commented Dec 23, 2019

vglavnyy commented Dec 23, 2019 • edited Loading

aardappel commented Dec 23, 2019 • edited Loading

no-more-secrets commented Dec 23, 2019

vglavnyy commented Dec 24, 2019

vglavnyy commented Dec 22, 2019 •

edited

Loading

vglavnyy commented Dec 23, 2019 •

edited

Loading

aardappel commented Dec 23, 2019 •

edited

Loading