L-Acoustics / avdecc

A set of open source libraries for controlling AVB entities using the AVDECC (IEEE 1722.1) protocol compliant to Avnu Milan Specifications
GNU Lesser General Public License v3.0
89 stars 22 forks source link

C linkage APIs cannot return a reference #148

Closed lhoward closed 9 months ago

lhoward commented 9 months ago

Commit c5a5d544 (I think) is causing some problems building on my Linux arm64 build host, specifically:

/home/lukeh/CVSRoot/padl/AVDECCSwift/Sources/CxxAVDECC/avdecc/src/bindings/c/protocolInterface_c.hpp:37:104: error: 'getProtocolInterface' has C-linkage specified, but returns user-defined type 'la::avdecc::protocol::ProtocolInterface &' which is incompatible with C [-Werror,-Wreturn-type-c-linkage]
LA_AVDECC_BINDINGS_C_API la::avdecc::protocol::ProtocolInterface& LA_AVDECC_BINDINGS_C_CALL_CONVENTION getProtocolInterface(LA_AVDECC_PROTOCOL_INTERFACE_HANDLE const handle);
...

I would have hoped that passing -a "-DCMAKE_COMPILE_WARNING_AS_ERROR" -a "--compile-no-warning-as-error" to gen_cmake.sh would have helped, but alas no.

lhoward commented 9 months ago

(Moreover presumably functions returning ref params shouldn't have C linkage...)

christophe-calmejane commented 9 months ago

Hi Luke, thanks for the info. You are right, while fixing bindings that were previously "hidden", some of same were actually invalid for the C language. I'll fix this as soon as I can find a bit of time. (I just noticed that my CI don't build the C bindings anymore, it looks like there are a lot more errors now).

christophe-calmejane commented 9 months ago

I wasn't able to reproduce this specific compilation errors using my linux, but anyhow these methods shouldn't have the external linkage flags, they are private methods. I'll fix it shortly, and I hope you'll be able to compile correctly.

In the future, I hope I'll be able to change the C (manual) bindings to swig bindings, like it's done for C# (python in coming).

lhoward commented 9 months ago

I don't doubt this is the correct fix but it does actually break my clang block wrappers which call getAggregateEntity(). I'll maintain a private fork with c5a5d544 and ab08c655 reverted for now, and try come up with a better fix down the track.

(The correct fix will be to use the C++ API directly, but I'm waiting for Swift's C++ interop to catch up. Perhaps by the time I need to use this code in anger it will have.)

christophe-calmejane commented 9 months ago

That's very interesting as both commit (together) merely change the defines that should never have been there, so it shouldn't be the cause of the recent compilation issues you have. Nevertheless, the file should not be included in a C project since it's really supposed to be an internal file used by the c++ code (but I do understand that you need to work around limitations of SwiftC++). If it does work for you with the 2 commits reverted, it shouldn't have any impact at all on the rest of the code so it's great if it's a workaround for you at the moment ;)

lhoward commented 9 months ago

Yeah, it’s more about symbol visibility, with the latest commit they’re no longer exported from the shared library.