[C++17] Add compile-time reflection for fields. #6324

no-more-secrets · 2020-12-10T02:23:38Z

This PR adds compile-time static reflection facilities to Flatbuffers' Tables and Structs in C++17 mode. Specifically:

The C++ generator has been modified so that, when in C++17 mode, it will emit Table and Struct field traits that can be used at both compile time and runtime jointly as a form of static reflection. This includes field types, field names, and a generic field-getter approach based on a tuple.
A comprehensive unit test that also serves as an example. It demonstrates the power of this reflection by implementing a full recursive stringifier for arbitrary Flatbuffers types, but without needing any runtime access to the *.fbs definition files; the computation is done using only the static reflection facilities introduced in this commit and is fully generic.
Built in C++17 mode and ran generate_code.sh, and included diffs in this commit. Only cpp17 generated files are affected.

Tested on Linux with gcc 10.2.0, but should work on all compilers.

Included in this commit is the following: - The C++ generator has been modified so that, when in C++17 mode, it will emit Table and Struct field traits that can be used at com- pile time as a form of static reflection. This includes field types, field names, and a tuple of field getter results. - Diffs to the cpp17 generated files. No other generated files are affected. - A unit test that also serves as an example. It demonstrates how to use the full power of this reflection to implement a full recursive JSON-like stringifier for Flatbuffers types, but without needing any runtime access to the *.fbs definition files; the computation is done using only static reflection. Tested on Linux with gcc 10.2.0. Fixes google#6285.

no-more-secrets · 2020-12-10T02:27:35Z

@aardappel @vglavnyy Please take a look; this should be exactly what we talked about in #6285.

The one thing that I wasn't sure about was the concern that you both raised that it "would be good to ensure fields_pack never constructs anything"; the fields_pack as I have implemented it just returns a std::tuple of light-weight things, such as pointers, references, ints, etc., and thus should not construct any objects or cause any performance concerns. But let me know if there is something that I'm missing.

Thanks in advance for your consideration :)

no-more-secrets · 2020-12-10T03:55:56Z

As you might be aware, there are a bunch of diffs to generated python files that are unrelated to my change... perhaps someone forgot to commit them recently? As far as I can see, all of the CI checks that are failing are doing so only for that reason. See PR #6325.

aardappel

Generally looks very good to me!

aardappel · 2020-12-11T00:11:44Z

tests/cpp17/stringify_util.h

+ * limitations under the License.
+ */
+
+// This contains some utilities/examples for how to leverage the static reflec-


This is great for testing.. I wonder if some of it can be generic enough that its worth sticking in include/flatbuffers ? Wonder what happens if someone actually wants to use this functionality, right now they'd probably to copy this.

Yes this is a standalone utility that someone could include and use in their own project if they are compiling with C++17. If you like I can move it into the include/flatbuffers folder so that they can include it more easily.

@vglavnyy do you think that makes sense?

I don't mind. However, we already have a lot of changes in C++ code: optional, span, --cpp-std c++11.
Maybe move it into the include folder after release?

Ok for now I will just leave it in the test folder, and we can move it at a later time.

Another thing to think about -- it is possible to parse this string and produce a real object with roughly the same amount of code/methodology. Perhaps we might want to include that as well when the time comes.

Another use case of this reflection is that we can provide a generic function called FlatbuffersEquals that will compare two Flatbuffers of the same type and tell you if they are equal or not. It will do this by iterating through the fields and comparing them, again using the static reflection. I believe traditionally this has not been possible to compare two buffers right? Or can you just do a memcmp() on the binary data? IIUC that would only work if the fields were added in the same order when creating the buffers?

aardappel · 2020-12-11T00:13:31Z

tests/cpp17/test_cpp17.cpp

+}
+
+const Monster* BuildMonster(flatbuffers::FlatBufferBuilder& fbb) {
+  using ::cpp17::MyGame::Example::MonsterBuilder;


why do we need another function creating a buffer? don't we have existing buffers we can test?

I wanted to use a Monster object for this test because it has many fields of all different types (including nested structs) in order to exercise all of the possibilities. However, in this particular test file (test_cpp27.cpp) there is not an existing function that creates a Monster object like this, so I have added this one. If there is another one somewhere else that you are aware of that I can reuse (which has all the different field types represented and populated) let me know and I am happy to reuse it.

CreateMonsterDirect + GetBuffer (or GetBufferSpan) + flatbuffers::GetRoot<Monster>?

I started changing to this approach (CreateMonsterDirect) but that seems a bit inconvenient in this case because it will force me to put many parameters corresponding to fields that I am not using, since I am populating a few fields towards the end, I cannot rely on the default values and I would have many parameters just nullptr or 0. Perhaps in this case where we are just populating a few select fields the Builder approach is better?

tests/cpp17/test_cpp17.cpp

…ffers into cpp17-field-traits

vglavnyy · 2020-12-13T06:42:19Z

@dpacbach
It doesn't compile with MSVC2019.
Commit 2046bff removed MSVC2017 and MSVC2019 from Appveryor-CI, so it passed CI but in fact, it doesn't work.
The Github action CI / Build Windows build uses MSVC2019 but it doesn't set FLATBUFFERS_BUILD_CPP17 option:

Run cmake -G "Visual Studio 16 2019" -A x64 -DCMAKE_BUILD_TYPE=Release .

Need to adjust https://github.com/google/flatbuffers/blob/master/.github/workflows/build.yml file.
Or adjust CMakeLists.txt to make FLATBUFFERS_BUILD_CPP17 option active if a modern compiler is used.

Update:
This is a possible fix for MSVC2019.
It fixes all warnings. Also StringifyVector reimplemented without using std::ostringstream.
This is only one of the possible options, I do not insist on it.

I've checked this code without Treat Warnings as Errors ('/WX`). This test failed, call stack added to commit message.

src/idl_gen_cpp.cpp

tests/cpp17/test_cpp17.cpp

vglavnyy · 2020-12-13T08:10:11Z

tests/cpp17/test_cpp17.cpp

+}
+
+const Monster* BuildMonster(flatbuffers::FlatBufferBuilder& fbb) {
+  using ::cpp17::MyGame::Example::MonsterBuilder;


CreateMonsterDirect + GetBuffer (or GetBufferSpan) + flatbuffers::GetRoot<Monster>?

no-more-secrets · 2020-12-13T09:10:27Z

@vglavnyy Thanks for helping me to fix the MSVC issues, I will make those changes that you suggest in your commit. But I am not clear on something that you said:

I've checked this code without Treat Warnings as Errors ('/WX`). This test failed, call stack added to commit message.

I did not see the call stack in your commit, how do I view it? But actually, I think there is a minor bug in your code at the end of the StringifyVector that would (at least) account for the test failure:

  text += "]";
  return text;
}

I believe that should be text += std::string("\n") + indent + "]"; right? Otherwise, the ] will be touching the final element in the vector. In any case, I will make your changes and test the fix and then we'll try again with MSVC.

Regarding the MSVC 19 C++17 build in the CI... do you want me to enable C++17 in that build.yml file in this PR, or will you just do the testing for me? Or do you want to enable it but in a separate PR? Thank you

vglavnyy · 2020-12-13T11:16:51Z

Comit 7575a2c (without changes)

flattests_cpp17.exe!flatbuffers::FlatBufferBuilder::NotNested() Line 1415 C++
flattests_cpp17.exe!flatbuffers::FlatBufferBuilder::CreateString(const char * str, unsigned __int64 len) Line 1535 C++
flattests_cpp17.exe!flatbuffers::FlatBufferBuilder::CreateString(const char * str) Line 1546 C++
flattests_cpp17.exe!BuildMonster(flatbuffers::FlatBufferBuilder & fbb) Line 70 C++
flattests_cpp17.exe!StringifyAnyFlatbuffersTypeTest() Line 109 C++
flattests_cpp17.exe!FlatBufferCpp17Tests() Line 242 C++
flattests_cpp17.exe!main(int __formal, const char * * __formal) Line 250 C++

Probably no one CI execute cpp17 tests.

Regarding the MSVC 19 C++17 build in the CI... do you want me to enable C++17 in that build.yml file in this PR, or will you just do the testing for me? Or do you want to enable it but in a separate PR? Thank you

For this PR fix of build.yml for Build Windows target will be sufficient to see that this code works well.

The current status of the c++17 code generator is experimental. But it would be better to enable compilation of flattests_cpp17 on all compatible CI to get more information from different compilers.
I will increase compilation time by about 30-60 seconds for each platform but I think it is acceptable.
The easiest way to do this - adjust CMakeListst.txt. This can be done late in a separate PR.

no-more-secrets · 2020-12-13T15:55:05Z

@vglavnyy update:

I enabled the C++17 build in MSVC 19 first and reproduced the warnings/failures. Note that I have included those build.yml changes in this PR.
I made @vglavnyy 's fixes to eliminate the warnings on MSVC (although I also made another change that no longer requires suppressing the "unreachable code" warning).
I fixed the test failure on MSVC (it was the dreaded NotNested error).
Now the windows CI builds all pass.
Implemented all of @vglavnyy 's other suggestions with the exception of the CreateMonsterDirect.
I added a fully_qualified_name field to the code gen that gives the also the namespace, which I'm sure will be needed in some cases since the table/struct name only can be ambiguous. The unit test now uses that fully qualified name.
I ran clang-format on all of the files I touched.

tests/cpp17/stringify_util.h

tests/cpp17/generated_cpp17/monster_test_generated.h

aardappel · 2020-12-14T21:30:26Z

Ah, wasn't aware that moving VS2017/19 to GitHub Actions removed the C++17 testing, sorry!

no-more-secrets · 2021-03-01T19:11:22Z

@aardappel We are ready for your review, PTAL when you have a chance. Just wanted to point out though that the broken CI check above appears to be coming from another change in master, I don't quite understand what it means. Apart from that, all the checks look good.

aardappel

This LGTM.
I wonder, since it generates a fair bit of code and it is somewhat niche in use, wether it should be behind a flag. After all, we have less niche uses of optional things behind a flag, e.g. --gen-object-api and --gen-mutable etc. It's not the biggest deal, but I know we have people relying on FlatBuffers being small in code size.

aardappel · 2021-03-01T21:44:28Z

src/idl_gen_cpp.cpp


    if (opts_.gen_nullable) { code_ += "#pragma clang system_header\n\n"; }

+    if (opts_.g_cpp_std >= cpp::CPP_STD_17) {


should this instead go into base.h or similar?

I think I'd probably avoid that, because the <tuple> header is a bit long and template heavy, and so we probably don't want to force users to include it unless they have this C++ static reflection feature enabled (with C++17).

no-more-secrets · 2021-03-02T18:09:19Z

@aardappel I'm fine to put it behind a flag, but I will let you guys decide since it won't affect me either way (I will always have that flag on). If you decide that you want a flag, let me know what you want to call it (I suggest perhaps --cpp-static-reflection). I guess that flag would then require that --cpp-std is at least c++17, and we'd have to add logic somewhere to enforce that.

no-more-secrets · 2021-03-04T14:24:59Z

@vglavnyy what do you think?

One thing that I would point out is that the majority of the C++17 features that we've added thus far (and the ones that I have an idea to add in the future, such as static reflection for enums which is now lacking) have to do with static reflection. So if we put those behind a flag, then the --cpp-std=c++17 flag effectively won't do much at all on its own without also enabling the proposed static reflection flag. That said, I do understand the principle of it, since theoretically in the future someone might add some C++17 specific code-gen that is not to do with static reflection.

vglavnyy · 2021-03-04T16:48:46Z

I think --cpp-static-reflection is the correct name for this feature.
This flag can set a opts_.cpp_static_reflection option if both --cpp-std=c++17 and --cpp-static-reflection are set.

aardappel · 2021-03-04T20:21:16Z

Yes, still think it would be good behind a flag. The use of C++17 is sure to expand in the future.

…pp17-field-traits

no-more-secrets · 2021-03-05T14:38:46Z

@aardappel @vglavnyy Ok I have added the flag --cpp-static-reflection, and I've put all of the C++17 stuff added thus far behind it, because it all happens to pertain to reflection (even the Create method from a past PR). Then in the idl_gen_cpp.cpp I have it check that if --cpp-static-reflection is enabled, then --cpp-std must be at least C++17 otherwise it fails with a flatc compile error. Also I have enabled --cpp-static-reflection by default in the generate_code.sh script otherwise the C++17 tests don't pass. PTAL

tests/cpp17/generated_cpp17/optional_scalars_generated.h

no-more-secrets · 2021-03-05T17:26:28Z

@aardappel I have addressed all of @vglavnyy 's suggestions and he has approved, and the CI checks look good as far as I can see (I think there is one failure there that is not related to my change), so now just waiting for your final approval and we can merge.

aardappel · 2021-03-05T18:01:37Z

Looks good, thanks!

vglavnyy · 2021-03-06T12:06:10Z

@dpacbach you forget to add --cpp-static-reflection to the generate_code.bat.
And probably flatc without --cpp-static-reflection doesn't generate Traits struct. I thought this flag should control only newly added code.

no-more-secrets · 2021-03-06T14:38:45Z

@vglavnyy I sent PR #6502 to fix the flag. And yes I did put the entire Traits struct behind the flag, because I felt that everything it contains is for the purpose of some kind of reflection, even the generic Create method. Do you think we should change that?

vglavnyy · 2021-03-06T14:51:04Z

This will break existing code that already depends on Create and type traits (I use Create).
Of course, it is possible to re-enable it again with --cpp-static-reflection but I don't feel that this the right way.
There are no reasons to break the working code.

no-more-secrets · 2021-03-06T14:55:01Z

@vglavnyy Ok I will send a PR shortly to restore it back to the way it was.

* [idl_parser] Add kTokenNumericConstant token This commit adds the new token for correct parsing of signed numeric constants. Before this expressions `-nan` or `-inf` were treated as kTokenStringConstant. This was ambiguous if a real string field parsed. For example, `{ "text_field" : -name }` was accepted by the parser as valid JSON object. Related oss-fuzz issue: 6200301176619008 * Add additional positive tests fo 'inf' and 'nan' as identifiers * Rebase to HEAD * Move processing of signed constants to ParseSingleValue method. * Add missed `--cpp-static-reflection` (#6324) to pass CI * Remove `flatbuffers.pc` from repository to unblock CI (#6455). Probably the generated flatbuffers.pc should not be a part of repo. * Fix FieldIdentifierTest()

no-more-secrets added 2 commits December 9, 2020 21:47

Fix int-conversion warning on MSVC.

a1f366c

Try to fix std::to_string ambiguity on MSVC.

c662260

aardappel reviewed Dec 11, 2020

View reviewed changes

Merge branch 'master' into cpp17-field-traits

a895c01

github-actions bot added c++ codegen Involving generating code from schema labels Dec 11, 2020

no-more-secrets added 5 commits December 11, 2020 13:55

Merge branch 'master' into cpp17-field-traits

2692175

Merge branch 'cpp17-field-traits' of ssh://github.com/dpacbach/flatbu…

4b458c2

…ffers into cpp17-field-traits

Fix clang-format diffs.

94790e2

Fix more clang-format diffs.

3f06697

Fix last clang-format diff.

7575a2c

vglavnyy suggested changes Dec 13, 2020

View reviewed changes

Enable C++17 build/test for VC 19 platform in CI.

181220d

github-actions bot added the CI Continuous Integration label Dec 13, 2020

no-more-secrets added 2 commits December 13, 2020 09:56

Forgot to add value to cmake command line variable.

5a42f2d

Various fixes/changes in response to @vglavnyy's feedback.

9f8ff70

no-more-secrets commented Dec 13, 2020

View reviewed changes

tests/cpp17/stringify_util.h Show resolved Hide resolved

vglavnyy reviewed Dec 14, 2020

View reviewed changes

tests/cpp17/stringify_util.h Outdated Show resolved Hide resolved

tests/cpp17/stringify_util.h Outdated Show resolved Hide resolved

tests/cpp17/stringify_util.h Outdated Show resolved Hide resolved

tests/cpp17/generated_cpp17/monster_test_generated.h Outdated Show resolved Hide resolved

no-more-secrets added 4 commits January 7, 2021 22:21

Merge branch 'master' into cpp17-field-traits

b5e7679

Replace "fields pack" with index-based getters.

44302e6

Fix MSVC error.

36ee3b0

Fix clang-format diffs.

09e46b2

Add fields_number field to Traits struct.

17e75b5

aardappel reviewed Mar 1, 2021

View reviewed changes

no-more-secrets added 2 commits March 5, 2021 09:11

Merge branch 'master' of https://github.com/google/flatbuffers into c…

883390d

…pp17-field-traits

Add --cpp-static-reflection flag and put those features behind it.

4bc3ca9

Fix clang-format diffs.

8d7b9dc

no-more-secrets requested review from aardappel and vglavnyy March 5, 2021 16:11

vglavnyy reviewed Mar 5, 2021

View reviewed changes

tests/cpp17/generated_cpp17/optional_scalars_generated.h Outdated Show resolved Hide resolved

Remove <tuple> include.

5102108

aardappel merged commit a69815f into google:master Mar 5, 2021

no-more-secrets deleted the cpp17-field-traits branch March 5, 2021 21:49

This was referenced Mar 5, 2021

[C++] Static Reflection of Enums #6499

Closed

[C++] Equality of Flatbuffers #6500

Closed

no-more-secrets restored the cpp17-field-traits branch March 6, 2021 00:32

no-more-secrets deleted the cpp17-field-traits branch March 6, 2021 00:33

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Mar 6, 2021

Add missed --cpp-static-reflection (google#6324) to pass CI

f375acc

no-more-secrets mentioned this pull request Mar 6, 2021

[C++17] Move Traits struct and Create method out of --cpp-static-reflection. #6503

Merged


		if (opts_.gen_nullable) { code_ += "#pragma clang system_header\n\n"; }

		if (opts_.g_cpp_std >= cpp::CPP_STD_17) {

[C++17] Add compile-time reflection for fields. #6324

[C++17] Add compile-time reflection for fields. #6324

Uh oh!

Conversation

no-more-secrets commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

no-more-secrets commented Dec 10, 2020

Uh oh!

no-more-secrets commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

no-more-secrets Dec 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vglavnyy commented Dec 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

no-more-secrets commented Dec 13, 2020

Uh oh!

vglavnyy commented Dec 13, 2020

Uh oh!

no-more-secrets commented Dec 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aardappel commented Dec 14, 2020

Uh oh!

no-more-secrets commented Mar 1, 2021

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!

no-more-secrets commented Mar 2, 2021

Uh oh!

no-more-secrets commented Mar 4, 2021

Uh oh!

vglavnyy commented Mar 4, 2021

Uh oh!

aardappel commented Mar 4, 2021

Uh oh!

no-more-secrets commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

no-more-secrets commented Mar 5, 2021

Uh oh!

no-more-secrets commented Dec 10, 2020 •

edited

Loading

no-more-secrets commented Dec 10, 2020 •

edited

Loading

no-more-secrets Dec 13, 2020 •

edited

Loading

vglavnyy commented Dec 13, 2020 •

edited

Loading

no-more-secrets commented Mar 5, 2021 •

edited

Loading