No more C support - Githubissues

Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.

MIT License

502 stars 194 forks source link

No more C support #127

Closed gummif closed 3 years ago

gummif commented 3 years ago

We are using a few years old version of this library and using it in a C codebase. I am now trying to upgrade to v2, but I see that C support has partially been remove (still some remaining ifdef __cplusplus), but the AdsLib.h and AdsDef.h do not compile any more. These are the things that break:

Enum type/size specifiers, e.g. enum ADSSTATE : uint16_t
Missing struct for struct names, e.g. AmsNetId netId in AmsAddr
The functions are no longer extern "C"
enum ADSDATATYPEID has been removed (any reason for that?)

I have a few questions, is C support? Will you accept PRs that fix these? Should we be using C++ instead since that looks like is better tested?

pbruenn commented 3 years ago

Hi gummif, well, the README states you need a C++11 compiler so we never intended to support C. Recently I dropped some C specifics, because I wanted to clean up a bit, and never thought any user still depends on C. My personal opinion is: "If there is a C++ compiler available for your platform use it!". There are many people out there telling you C++ is bloated and has to much overhead. My opinion is this is only true for compile times. C++ doesn't force you to use all of its features. Of course you need discipline to limit yourself to zero overhead features (like sized enums), but I think that's worth it and you can be much more productive in C++ than in C. In the worth case keep writing C and benefit from compiler optimizations, which you pay with longer compile times. About the ADSDATATYPEID, I think this was an accident. Could you spot me to it or send a PR?

gummif commented 3 years ago

Thanks for the comment. The ifdef __cplusplus are confusing and I would suggest to clean them up (also comments in this recent PR https://github.com/Beckhoff/ADS/pull/119 suggested that the C API was still available). Using C++ is not a problem since we have a mixed C and C++ codebase, but the part dealing with ADS was written in C, so we will have to do a rewrite in C++ to upgrade.

I'm not able to find that enum in the repo, could be something we patched locally.