Generated header can cause warning: integer constant is so large that it is unsigned

[JohanNicander]

The problem occurs when using a large enough integer as a default value in fbs for an ulong (and possibly other unsigned types?) and generating JSON parser and printer (again possibly others?). The code generator will produce for example if (val != 18446744073709541616 || (ctx->flags & flatcc_json_parser_f_force_add)) { for the parser and flatcc_json_printer_uint64_field(ctx, td, 10, "age", 3, 18446744073709541616); for the printer; but it needs to use a correct literal form which I think could be 18446744073709541616U.

[mikkelfj]

Thanks for reporting. The correct form should UINT64_C(18446744073709541616). The is consistently used elsewhere, but apparently it slipped here. Your suggestion would not be portable.

[mikkelfj]

If you need an urgent fix, you can try add this here: https://github.com/dvidelabs/flatcc/blob/d875e6aebb64156a292bb9f494f57cdd0bb5f9b4/src/compiler/codegen_c_json_parser.c#L610-L615

There is also a potential issue with float vs double if the compiler should start complaining about precision loss, but I don't think so.

[mikkelfj]

A better fix is to get the actual type of the val variable and use the appropriate const macro.

[mikkelfj]

Please check this branch, and let me know if you find any other similar issues.

The code actual got better and simpler by using a print_literal function that wasn't available when the parser generator was originally written.

https://github.com/dvidelabs/flatcc/tree/defaultjsoncheck

It is the same function that is used to assign values to enum symbols.

[JohanNicander]

I can confirm that I get the same errors using that branch as well.

[mikkelfj]

Can you provide more details. I might be that something was fixed but something else wasn't. Or it could be that the new UINT32C etc. macroes somehow complains on too large constants (which shouldn't happen).

[mikkelfj]

If you get the exact same errors, then please check your build chain and look at the JSON parser output. I checked in my test that the output changed to use UINT32_C(...) wrappers. If you don't see those, either the branch or your build is wrong.

This is the first line containing force_add in the JSON test parser:

if (val != INT32_C(0) || (ctx->flags & flatcc_json_parser_f_force_add)) {

found in

./build/Debug/test/json_test/generated/monster_test_json_parser.h

after runnings scripts/test.sh

[mikkelfj]

I forgot to fix the printer. But please double check the parser. I'll update the branch with the printer fix and update here when done.

[JohanNicander]

My mistake! The parser correctly encapsulates the default value.

Thank you for your fast help!

Will this be incorporated into master later on? If yes, how soon would you estimate?

[mikkelfj]

This will go on master as soon as you help confirm the JSON printer fix. I have currenlty implemented most of this and will probably push to branch in a few hours or tomorrow.

[mikkelfj]

@JohanNicander Please review printer. Pushed to branch.

[mikkelfj]

Note that the printer did assign U and ULL suffixes to some constants, though not all, but as stated, this is not really portable, so I have replaced these with wrapping const macros here as well (UINT32_C etc.).

[JohanNicander]

Yes, your fixes seem to have worked!

[mikkelfj]

Branch merged to master and deleted.