Fix a bunch of compiler warnings & linking errors

[ximion]

Hi everyone!

While working on the Syntalos plugin, I found a bunch of issues due to its quite strict compiler flags, that would be nice to address upstream:

• missing-prototypes: Make clear that some functions are never exported by declaring them as static. This also fixes linking errors on newer versions of GCC.
• shadow: Do not shadow variables, this makes code a bit cleaner and easier to read.
• unused-but-set-variable: Drop unused-but-set variables, no need to compute values we don't use (and this will be optimized out anyway).
• Some spurious whitespace fixes

The first issue actually fixes a pretty annoying linker issue:

/usr/bin/x86_64-linux-gnu-ld.bfd: /tmp/cc2Uc0Ru.ltrans0.ltrans.o: in function `oni_ft600_reset_acq':
syntalos/build/dev/../../vendor/liboni/api/liboni/drivers/ft600/onidriver_ft600.c:322:(.text+0x35d): undefined reference to `oni_ft600_reset_ctx'
/usr/bin/x86_64-linux-gnu-ld.bfd: /tmp/cc2Uc0Ru.ltrans0.ltrans.o: in function `oni_driver_create_ctx':
syntalos/build/dev/../../vendor/liboni/api/liboni/drivers/ft600/onidriver_ft600.c:366:(.text+0x45a): undefined reference to `oni_ft600_reset_ctx'
/usr/bin/x86_64-linux-gnu-ld.bfd: /tmp/cc2Uc0Ru.ltrans0.ltrans.o: in function `oni_driver_write_config':

This is caused because the ft600 driver declares stuff like oni_ft600_reset_ctx as inline, so when the compiler chooses not to inline a call site there is no external symbol to link against. I can workaround that by switching to C89 (GNU89) inlining, which does emit an out-of-line definition as well, but that kind of sucks. This project can easily use C11 or an even newer C standard.

This patch fixes the linker issue as well by solving the missing-prototypes warning by declaring internal functions as static. That way, the compiler can make better optimization choices and the linker issue goes away as a bonus.

Long-term, it might make sense to gather all explicitly exported stuff in a header and set explicit visibility, but i wanted to keep the changes minimal for now.

On the r = ctx->max_read_frame_size % align; change: It feels like the intent was to validate the result here, but that has not been done. This felt safe to remove, but if there is some validation that should be done instead, please let me know!

[aacuevas]

Thank you for the commit!

The gcc version we have been using (admittedly far from the latest) did not report these warnings, but they are indeed the better practices.

As you point out, all internal functions should be static, thanks for the catch. As a note, the header that defines which functions need to be exported by an onidriver are in onidriver.h.

Regarding the r variable, it's a leftover from a previous way to check the alignment, we do it in a bitwise way in the lines just below the ones you deleted, they can be removed safely.

I will fix some conflicts with another internal commit we just merged and merge this PR.

[ximion]

Thank you! :-)

On the Syntalos side, one thing that makes integration a little bit difficult is that this project statically embeds an x86_64 version of libftd3xx-static.a and the use of direct Makefiles for building.

As far as I can see, the sources for the library are actually available, and I already wrote a bit of Meson integration for easy (cross)building.

If I were to do the work, would you accept a patch that builds everything on the FT600/liboni side from source, using Meson, with CI integration to create Windows/Linux binaries on GitHub? (using MSYS2 for the former, this would still allow you to just fetch binaries for other OpenEphys components instead of using the sources)
It could be used alongside the existing Makefiles, but of course I won't do that work if it doesn't fit your plans.

[ximion]

The gcc version we have been using (admittedly far from the latest) did not report these warnings, but they are indeed the better practices.

This was built using 15.2.0 using an array of extra flags, so some of them wouldn't actually show up by default (built using -Wall -Wextra -Wcast-align -Wno-uninitialized -Wempty-body -Wformat-security -Winit-self -Wmaybe-uninitialized and a few more).

Things going via onidriver.h makes sense, I was wondering how that would work on Windows otherwise which needs explicit exports :-)