search

Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.
MIT License
502 stars 194 forks source link

Usage of memcpy overwrites whole class objects #114

Closed tangram67 closed 1 year ago

tangram67 commented 4 years ago

Hi to all.

Thanks for making the ADS source code available to the public domain. I integrated the the source files to my framework and I found some inconsistencies mainly for AmsNetId which might be caused by porting C structs to C++ classes. memcpy is used to copy (and memcmp to compare) the contents of the whole class insted of the NetId only. I reworked the assignement and compare operators of the AmsNetID class. I attached a DIFF file with my changes to the original source code. I also added a namespace "ads" to avoid name collisions when integrationg the sources instead of linking against a static library. The size of the library files are not that large, so it might be useful, also for others, to use them directly. And in this case a separate namespace might be useful.

diff.txt

pogojotz commented 3 years ago

Hi @tangram67, I have some points of critique with your suggestion:

  1. You are suggesting two very different things in one diff. This makes it difficult for a reviewer/implementer to tell them clearly apart.
  2. You should use the Github workflow for Pull requests to suggest changes. This makes it MUCH easier to review and discuss those in detail. See https://guides.github.com/introduction/flow/ on how to do this properly.
  3. Changing the namespace of a complete library is no small thing. Everybody who is using that library has to update their code if they want to go with the next release.
  4. The default output of diff is kind of hard to read for humans. Using the flag diff -u produces the "unified" format output with 3 lines of context. This seems to be most widely used and understood in the open source world.
  5. Last but not least: stick to the code style ;) Even for the things like operator < (... vs operator<(...

I suggest you make a proper pull request with the actual functional changes you suggest, without the namespace part, because that is a completely unrelated thing. If you need help with the process, please tell so.

tangram67 commented 3 years ago

Hello Jürgen.

I created a pull request https://github.com/Beckhoff/ADS/pull/119 with the functional changes.

As you mentioned I removed the changes for introducing a namespace. I also think that introducing a namespace is a breaking change for the library. But I still think that is is worth to think about in general. But I do fully understand the disadvantages.

marcus-sonestedt commented 2 years ago

The namespace can often be fixed with a one-liner, i.e. "using namespace ADS;" .. but not always (if someone is discriminating between global vars with ::AdsDevice and their own local-namespaced AdsDevice for instance).

I do think using namespaces is a good idea, but having all the names prefixed with "Ads" works too, so not a strong opinion. My 0.02€. :)

Maybe close this issue as we have a PR on it?

pbruenn commented 1 year ago

discussion moved to https://github.com/Beckhoff/ADS/pull/119 and was closed there a long time ago