epics-base / pvDataCPP

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

Replace display.format with .form and .precision #62

Closed mdavidsaver closed 5 years ago

mdavidsaver commented 5 years ago

Update StandardField::display() to replace 'display.format' with 'display.form' and 'display.precision' as in https://github.com/epics-base/pva2pva/issues/19 .

I think the main question here is where else this change needs to be recorded.

The accessors for 'display.format' are removed from class PVDisplay, but new accessors are not added as this class may be deprecated (https://github.com/epics-base/pvDataCPP/issues/61).

This change will effect all external code using the result of StandardField. Either directly, or indirectly through normativeTypesCPP.

@ralphlange @anjohnson @mrkraimer

@shroffk A corresponding change to pvDataJava would be advisable, though I see no particular need to synchronize these.

mdavidsaver commented 5 years ago

Also, in the spirit of highlighting the boring work I do. 4c736077994dec6271c08717b16a880d924c6123 2814c779bd9c3f13e70b95df40c9aa3320272028

ralphlange commented 5 years ago

Appreciated.

mdavidsaver commented 5 years ago

I'm undecided on when to apply this change. It isn't urgent for me as I don't use StandardField myself, and I find that caProvider and testServer in the PVA repo crash due to unchecked getSubField() calls. This will be fixed when I push mdavidsaver/pvAccessCPP@5b14bbef5aeec0dfbce26e48542a6b3469ee4d13 to this repo.

anjohnson commented 5 years ago

Would it be feasible to support both the old and new fields for at least one release? I understand that would need more work and require some extra code to convert from one format to the other (reasonable efforts only), but I don't see any obvious difficulties with doing that. By deprecating but still providing the old fields we can warn developers without requiring them to update their code immediately after we release.

BTW, I don't see any obvious way at compile-time for external C++ code that has to build against both old and new APIs to detect which one to use, other than to look at the release version (which doesn't work when someone is building against the latest git repo and we haven't released yet). I realize that a client needs to detect the fields that are present at run-time, but at compile-time an application still has to know not to call the getFormat() and setFormat() methods when it's built against the new API.

mdavidsaver commented 5 years ago

Would it be feasible to support both the old and new fields for at least one release?

Yes, although this seems like kicking the can down the road as it does not address the underlying question of whether removing display.format would cause problems.

I'm specifically referring to the type definition here. FTBFS caused by changes to PVDisplay would be readily apparent. I'm more concerned about runtime problems caused by eg. unchecked getSubField(), which seems to me to be endemic in the code which uses StandardField (as well as NTCPP).

BTW, I don't see any obvious way at compile-time for external C++ code that has to build against both old and new APIs to detect

I'm not inclined to spend time on this logic unless at least one external user can be identified who would use it. Are you willing to add this?

mdavidsaver commented 5 years ago

This wasn't discussed at the recent core developers meeting. I'm not sure the right people were in attendance anyway. I would like to get some feedback from a user application. Absent this I suppose I must conclude that there are no (deliberate) users, and merge anyway.

So, unless some negative feedback comes in by 22 April (2 weeks), I'll consider that silence is acceptance.

mdavidsaver commented 5 years ago

Merging has lead to downstream failures in epics-base/normativeTypesCPP#15 which turns out to be even stricter than I had realized.

mdavidsaver commented 5 years ago

Reverted with cd2436342d71b173eb4377e61d396e348c15546a