Replace memcpy and memcmp with assignement and compare operators

[tangram67]

Replace memcpy and memcmp with assignement and compare operators in class AmsNetId.

[pogojotz]

This is a public header, which should be usable in pure C projects. Without this guard a C compiler will complain.

[pogojotz]

Mixed usage of tab and spaces for indentation. Please replace all instances of tabs (there are other lines too) with 4 spaces.

[pbruenn]

We have an uncrustify config for this:
https://github.com/Beckhoff/ADS/blob/master/tools/uncrustify.cfg
make uncrustify

[pogojotz]

I don't think there is a need to explicitly add a destructor here.

[tangram67]

Missing destructor causes warnings in Eclipse, but I will remove it.

[pogojotz]

Suggested change

	bool AmsNetId::operator == (const AmsNetId& rhs) const
	bool AmsNetId::operator==(const AmsNetId& rhs) const

... also for the other operators

[tangram67]

Hi pogojotz.

I followed your suggestions and updated my pull request.

[pbruenn]

This merge request is hard to read, please squash your updates, if they are fixes only.
Besides I don't get the purpose of this change. In general you should elaborate that purpose in the commit messages.

[tangram67]

The reason for my pull request was to avoid using memcpy on class pointers, see changes in file AmsRouter.cpp (line 116):

memcpy(&pAddr->netId, &localAddr, sizeof(localAddr));

My pull request "Replace memcpy and memcmp with assignement and compare operators" is suggesting a fix by using assignment operators to copy the class/struct member "uint8_t b[6];" content.

[pbruenn]

I still don't see the reason or benefit. You are adding more than 40 lines of code to replace one call of memcpy with an assignment? AmsRouter::localAddr is a struct AmsNetId, which itself is just an alias for an uint8_t[6]. It has no vtable so sizeof(struct AmsNetId) is just the same as sizeof(uint8_t[6]).
What am I missing?

[tangram67]

I believe the code will/might work, as long as the C-style struct does contain native C variable types like the given uint8_t[6] and as long as both objects are persistent. By copying the whole struct (class) content by using memcpy on the object pointers, also the C++ member function pointers are copied from one object to the other. This leads to the situation that the copied object is accessing (or lets say is working) with the member methods of the original object.
The assignment operator from my PR avoids this situation by copying the data content of the struct only. For me, this looks like a cleaner solution.

[pbruenn]

I think clean depends on the view. I my view it's a big patch (4 files changed, 70 insertions(+), 14 deletions(-)) fixing a theoretical issue. AmsNetId is part of the ADS C API so I am pretty sure it won't get any vtables.
I appreciate your effort and the virtual destructor for the Router interface might be correct. But I am not yet convinced by this merge request

[tangram67]

Hmm, hard to comment...
There is a common sense that only POD data objects are allowed to be copied by memcpy. Memcopying class objects is a risky business, the result depends from case to case. I think, that relying on indifferent prerequisites is never a good design choice.
But you're free to do so and therefore I will close the request for now.