-
Notifications
You must be signed in to change notification settings - Fork 8
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
isCompatible() considered harmful #15
Comments
@MarkRivers @brunoseivam I think this would impact AD. |
Hi Michael, I think (in AD's case at least) that the field order does not matter. The code is just testing that the received PVStructure is, in fact, conformant to the NTNDArray definition. All fields are accessed by name thereafter. I believe field order should be considered an implementation detail and should not be relied upon regardless. At the same time, I understand that this would mean having to rewrite all |
Also, as I think more about it, the change you are proposing to the structure would be a breaking change anyway, right? Will the "version" of the normative types be bumped? Maybe |
Better make that "proposed" in the past tense. epics-base/pvDataCPP#62 was created back in January, and merged last night. Which then triggered various CI failures.
How would this be used? (and who would do the work of implementing?) Anyway, I don't see how it helps avoid a compatibility break. With epics-base/pvDataCPP#62 merged, even if |
The way I see it, Changing the |
Fixing
isCompatible()
is proving quite tedious because of code like the following. note thefield[3]
, which is asserting not just the existence of string 'display.field', but that it is the 4th of 5 fields.This makes even the slightest change to the
display
substructure a compatibly break, and would tie my hands wrt. replacing 'display.format'!Given this, and the grief caused by returning false with no indication of which test has failed, I'm inclined to stub the
is*()
methods to always return true. Code calling methods documented to return NULL really should be checking for this case (though I know this is often not done).normativeTypesCPP/src/ntfield.cpp
Lines 130 to 134 in 2d186d4
The text was updated successfully, but these errors were encountered: