dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
632 stars 180 forks source link

Doesn't compile with mingw32 gcc on windows with -std=c99 #173

Closed Elkantor closed 8 months ago

Elkantor commented 3 years ago

On windows, when using gcc with the ming32 toolchain, it doesn't compile:

make
[  6%] Built target flatccrt
[  7%] Building C object src/compiler/CMakeFiles/flatcc.dir/__/__/external/hash/cmetrohash64.c.obj
[  8%] Building C object src/compiler/CMakeFiles/flatcc.dir/__/__/external/hash/str_set.c.obj
[  9%] Building C object src/compiler/CMakeFiles/flatcc.dir/__/__/external/hash/ptr_set.c.obj
[ 10%] Building C object src/compiler/CMakeFiles/flatcc.dir/hash_tables/symbol_table.c.obj
In file included from D:\Projects\flatcc\src\compiler\hash_tables\symbol_table.c:2:0:
D:/Projects/flatcc/src/compiler/symbols.h:156:6: error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]
     };
      ^
cc1.exe: all warnings being treated as errors
make[2]: *** [src/compiler/CMakeFiles/flatcc.dir/hash_tables/symbol_table.c.obj] Erreur 1
make[1]: *** [src/compiler/CMakeFiles/flatcc.dir/all] Erreur 2
make: *** [all] Erreur 2
mikkelfj commented 3 years ago

Have you looked at: https://github.com/dvidelabs/flatcc/pull/171 There is ongoing work on updating the CMake system and also add GitHub actions compilation which includes a new mingw build cross and native.

I'm not claiming this solves your issue, since I don't recall fixing any unnamed unions. Maybe the mingw build only builds the runtime target?

FlatCC attempts to avoid unnamed unions because of portability issues so this likely needs to be fixed in the symbol table.

Elkantor commented 3 years ago

Thanks for the link, I'll try by going on the branch of the PR to test if that fix the issue. Actually yeah, I guess the real fix should be to rewrite the structs which contain nested struct or unions ;)

mikkelfj commented 3 years ago

You can see the mingw build here:

https://github.com/madebr/flatcc/runs/1596243638?check_suite_focus=true

Note that I had to make a few changes to flatcc before it would compile, but these are on the master branch now.

mikkelfj commented 3 years ago

Also note that there is a printf formatting issue that must be fixed in the CMakeList.txt file or whatever you use to build. A warning must be disabled. This has not been merged to master. You can find it in the PR discussion.

mikkelfj commented 3 years ago

This union type fb_value_t is used everywhere, and the number of named indirections is already long when accessing values in the symbol table. So this is not an easy fix.

I suggest either removing -Wpedantic from the MinGW build, which should also fix MS formatting warnings, or use -std=gnu99, or -std=c11, noting that c11 officially supports anonymous unions. If you do keep -Wpedantic, you need to add -Wno-pedantic-ms-format specifically for MinGW targets.

While I do want to support -Wpendantic wherever practically possible, it has also been disabled in FlatCC for the latest GCC versions because it became nearly impossible to silence an increasing flood of new warnings while maintaining portable code.

There is currently no MinGW detection in the CMakeList.txt but there is in a PR as mentioned earlier. Here -std-c11 and -Wno-pedantic-ms-format is configured for MinGW.

mikkelfj commented 3 years ago

Alternatively you can choose to build with FLATCC_RTONLY to limit the build to the runtime library. It should not depend on anonymous unions (hopefully).

mikkelfj commented 8 months ago

Closing as won't fix. It is not practical to change the root cause, the anonymous union in fb_value and generally flatcc does not support pedantic warnings from gcc (as they work against portability). Following next release there will be some updates to the build system, and if there is explicit interest in mingw support, please add a PR for that with suitable flags.