Code review comment for ~dirk.zimoch/epics-base:fix_zero_size_arrays

Revision history for this message
Andrew Johnson (anj) wrote :

On changing the semantics of passing pnRequest=NULL to dbGet(): I like this as a solution, it probably just clarifies existing practice but I haven't thought through if and how any other code might have to change (if at all). Presumably similar changes apply to the semantics of the dbGetField(), dbGetLink(), lset::getValue() etc. since they normally pass that argument through to dbGet().

Has anyone done a search through the epics-modules repo's on GitHub to see if there is any code there which might need changing? Hopefully there isn't any such code, but I would like to be proactive about that just in case. If there might need to be any changes to external code we should create a HAS_EMPTY_ARRAY_SUPPORT macro in dbAccessDefs.h to allow for easy compile-time detection.

Do recent versions of autosave properly save & restore an empty array field? There are other tools out there for doing the same thing, but I believe the synapps autosave module has the widest usage.

I see the change to modules/ca to allow the ca_array_put() API to write a 0-length array, do we need to make any changes to the caput program to allow that to be exercised from the command-line? That might be reasonable as a separate development for later, but we should support it.

Finally a minor code complaint: I don't like the use of the epicsOldString type. I know it's just a typedef but it isn't by accident that there is only one existing use of this data type in the IOC code and that's in newer code where sizeof(epicsOldString) should be replaced by MAX_STRING_SIZE anyway. The epicsTypes.h header even has a comment block above tye typedef which says "Dont use this - it may vanish in the future". I read that imperative as applying to the typedef and not so much to the MAX_STRING_SIZE macro (which would be much harder to get rid of) but there are several other ancient typedefs, enums and arrays in epicsTypes.h which should really get dumped.

« Back to merge proposal