Use an unsigned type for public interface flags

[bjosv]

When using the provided API to set multiple JSON-printer flags via flatcc_json_printer_set_flags() some compilers and tools will give errors due to a bitwise operation on a signed integer type.

The provided flags are an enum and in C enums are signed (C99 6.4.4.3p2). Since C doesn't mandate how to represent a negative signed value (one's complement/two's complement/..) there is no portable way to do bit operations on signed values.

When a user wants to set multiple flags using bitwise OR with the provided flags this gives errors in the user code indicated by tools like clang-tidy. See the checker: https://releases.llvm.org/10.0.0/tools/clasng/tools/extra/docs/clang-tidy/checks/hicpp-signed-bitwise.html

This PR proposes the use of unsigned int flags instead of an enum, which avoids having problems in the usercode when using this library.

An example project that triggers the issue: https://github.com/bjosv/flatcc-experiments/tree/main/issues/enums see comment.

A C++ reference regarding the use of unsigned types for bit manipulations: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-unsigned

[mikkelfj]

From memory I think this issue has been up before. If I remember correctly enums are so widely used in flatcc that it is not practical to change to unsigned constants. Fixing the JSON printer would in that case only be a band aid.

There should be an issue discussing this somewhere.

[mikkelfj]

You can see an example below:

The solution is to cast wherever logical operations are applied is shown in the comment. Note that this isn't cosmetic - it is potentially undefined behavior or something along those lines.

https://github.com/dvidelabs/flatcc/issues/231#issuecomment-1122449195 quote

index 96d4b94..09b98d3 100644
--- a/src/compiler/semantics.c
+++ b/src/compiler/semantics.c
@@ -617,7 +617,7 @@ static int analyze_struct(fb_parser_t *P, fb_compound_type_t *ct)
     }

     ct->symbol.flags |= fb_circular_closed;
-    ct->symbol.flags &= ~fb_circular_open;
+    ct->symbol.flags &= (uint16_t)~fb_circular_open;
     ct->order = P->schema.ordered_structs;
     P->schema.ordered_structs = ct;
     return ret;```

[mikkelfj]

Note: I would not mind a PR that fixes this, but it is presumably a LOT of work to cover all the enums.

[bjosv]

If I remember correctly enums are so widely used in flatcc that it is not practical to change to unsigned constants.

I have seen the use of enums in many places, but I guess the issue is only given when it's used as a flag by the user, which I haven't seen that many cases of. Possibly when using flatcc_builder_buffer_flags which we haven't used, but maybe that is another candidate that also should be changed in this case..

edit: ..and flatcc_json_parser_flags

[mikkelfj]

Well, a limited fix to user facing flags would be fine. After all, internal use can, and have, been handled with casts.

Of course it might affect users as the interface would break slightly, but that is OK.

We just need to make sure we find all the cases. Both JSON printer and parser at least, and probably several places in the builder / emitter / allocator. I.e. inspection of all header files in include/flatcc.

[mikkelfj]

As to your PR, I think the proposed changes are fine as far as they go. I am just wondering if we should use a fixed size type instead of unsigned int since unsigned int could technically be too small for some values on some (hypothetical) compilers. We could also consider adding a typedef for the flag type. The generated code with flatbuffer schema enums does something similar.

[bjosv]

Sounds good, I can add commits with changes for flatcc_builder_buffer_flag and flatcc_json_parser_flags to this PR as well, unless you want to have it separate or do it yourself. There might be more that these two, but I'll take a deep dive..

For the type I'm open for suggestions if you prefer any, or have some previous pattern.

[mikkelfj]

Please go ahead. I think one PR is fine - if it becomes huge, maybe more would be needed. Note that I generally squash the PR into master.

[mikkelfj]

If you have 3 clean commits for printer, parser and buffer I would use those over squashing.

[mikkelfj]

Wrt type I'm thinking something like typedef uint32_t flatcc_json_parser_flags_t; but may also be uint16_t depending on the type used internally. If values are close to limit the type may have to be larger for future extension.

[bjosv]

I could only find the printer, parser and buffer enums which now are replaced.

The buffer flag has a size dependency to the internals but should be covered by a static assert (which seems to be covered the portability layer), but the placement of it can maybe discussed..

[mikkelfj]

Yes, I looked through the code, can say if it is all correct, test must confirm that, but it looks solid. I also noticed the static assert. I'm not sure if it is necessary and if the null pointer cast is sufficiently portable.

Note: if you can the run the test with clang and gcc on Linux it would be good since only appveyor windows CI/CD is currently working. I'll check MacOS.

[mikkelfj]

scripts/test.sh

[bjosv]

I have run fine with:

11333  2022-10-18 18:30:33  ./scripts/test.sh\n
11334  2022-10-18 18:30:50  CXX=clang++ CC=clang ./scripts/test.sh\n

on Linux (Ubuntu 22.04.1 LTS) with:

• Ubuntu clang version 14.0.0-1ubuntu1
• g++ (Ubuntu 11.2.0-19ubuntu1) 11.2.0

[mikkelfj]

MacOS clang 13 also fine. Thanks for working on this. Any sugg. on static assert before merging?

[bjosv]

I'm not that familiar with appveyor, is it better than githubs CI? Might have to look into that.

Regarding the assert then I was suddenly thinking if it should be moved away from the "public" headers that users includes, since its an internal thing. Maybe it can trigger warnings in usercode by some static tool.. But its nice to have something since I add the cast, which would hide a single handed changes in the future.

[mikkelfj]

Well, we can leave until someone complains ...

Appveyor is for historical reasons - the only way to test on Windows. Travis was used fo MacOS & Linux Clang & GCC, but they changed their API/URL and I never got to update.

The plan is to move to Github now that that is available. There is a vacant position for that ...

[bjosv]

Sure, sound good regarding the assert. I ran clang-tidy on it with big bunch of checks and current seems fine.

Aha, yes, a lot of projects had to move from travis. Github supports windows and mac as well.. so if I'm bored by other things then I know where to look :)

[mikkelfj]

Well, if you test really old Visual C compilers (as we do), Appveyor is better than Github actions, but maybe those should be retired. I think MSVC 2010 can only go on GH Actions if you do something extreme like installing it manually.

[bjosv]

Ah ok, windows is not my strong side, have only setup simple jobs..

Thanks for your effort with this project, and being so quick to respond!

[mikkelfj]

Thanks. Anyway, a GH build for just Linux would also be valuable.