search

epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

bitSet: macros and vector storage #28

Closed mdavidsaver closed 8 years ago

mdavidsaver commented 8 years ago

Use std::vector to manage storage instead of raw array. Make some global variable "constants" into macros so they can be inlined. Add swap() method.

Resumption of #22

dhickin commented 8 years ago

The implementation looks correct and using a vector is obviously a better approach than hand-rolling the array management. (I guess the reason for the original implementation was that it was a copy of the Java code.) BYTES_PER_WORD and WORD_OFFSET are also an improvement.

swap() looks like a useful addition. Is there a danger of mixing up with the shared_ptr version though? I.e. bitset1.swap(bitset2) versus bitset1->swap(*bitset2) for BitSetPtrs bitset1/2.

One comment/question though: Was it necessary to replace the constants and inline function with macros? The former are usually preferred to the latter and the compiler can still inline the constants.

mdavidsaver commented 8 years ago

Is there a danger of mixing up with the shared_ptr version though?

Yes, somewhat. As your example shows, it would take two typos to trigger ('->' for '.' and '*') since there are no implicit casts between BitSet and shared_ptr<BitSet>.

If you worry about this, consider the return types of PVUnion::get() and shared_ptr<PVUnion>::get(). This gave me some trouble when adding unions to pvaPy.

Was it necessary to replace the constants and inline function with macros?

Force of habit mainly. I suppose with c++11 and 'constexpr' this pattern will eventually go away.

dhickin commented 8 years ago

Perhaps a more realistic issue is the behaviour of bitset1.swap(bitset2) depending on whether bitset1/2 are pointers or references. I guess the caller just has to be careful.

(Incidently I did worry about PVUnion issue - I raised the name clash back in the F2F in Ljubljana 3 years ago. It was this that made me think of the issue with swap. I guess the resolution is the same - don't make the mistake.)

Would you have any issue with replacing the macros with constants/inline functions? A quick experiment with an out-the-box build of pvData on my Linux system and an object dump shows it makes no difference replacing macros with constants/inlines in this case.

mdavidsaver commented 8 years ago

Would you have any issue with replacing the macros with constants/inline functions?

No.

Were I the original author, I wouldn't have bother with definitions for these as I don't find X&63 and X>>6 sufficiently mysterious or magical. Since they were there, I kept them.