Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 6, 2019

Conversation

mdavidsaver
Copy link
Member

Update StandardField::display() to replace 'display.format' with 'display.form' and 'display.precision' as in epics-base/pva2pva#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 (#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.

Remove instance accessors w/o replacement
as a prelude to deprecation.
@mdavidsaver
Copy link
Member Author

Also, in the spirit of highlighting the boring work I do. 4c73607 2814c77

@ralphlange
Copy link
Contributor

Appreciated.

@mdavidsaver
Copy link
Member Author

mdavidsaver commented Feb 20, 2019

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@5b14bbe to this repo.

@anjohnson
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@mdavidsaver
Copy link
Member Author

Reverted with cd24363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants