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

Field sub-class comparison is incomplete #52

Open
mdavidsaver opened this issue Feb 7, 2018 · 11 comments
Open

Field sub-class comparison is incomplete #52

mdavidsaver opened this issue Feb 7, 2018 · 11 comments
Assignees

Comments

@mdavidsaver
Copy link
Member

When the specializations BoundedScalarArray, FixedScalarArray, and BoundedString were added, the comparisons (operator==) were not updated. So only the element/scalar type is to decide on type equivalence.

I'm not sure what the technical effects of this omission are, but given that no code seems to use these it's minor.

@mdavidsaver mdavidsaver added the bug label Feb 7, 2018
@mdavidsaver mdavidsaver self-assigned this Feb 7, 2018
@mdavidsaver
Copy link
Member Author

@ralphlange reports that a crash, probably due to this issue, has been observed at ITER when two FixedScalarArray arrays are used with differing sizes. This is with 7.0.1.1

@mdavidsaver
Copy link
Member Author

Given that this feature has had ~zero use in the past 4 years, I'm inclined to deprecate it. That is, not just the class definitions, but eventually the protocol feature. Of course this isn't a step to be taken lightly, so it would need to be done in steps.

  1. Mark BoundedScalarArray, FixedScalarArray, and BoundedString as deprecated
  2. Hide the implementations. This would prevent a node from creating structures with these types, but allow it to handle definitions received from a peer.
  3. With a minor version bump, begin disallowing for newer versions
  4. Drop support along with minor version 1 (probably as I retire)

@mdavidsaver
Copy link
Member Author

Marked "help wanted" as I'm not inclined to spend time fixing this feature.

@ralphlange
Copy link
Contributor

ralphlange commented Oct 17, 2018

@ralphlange reports that a crash, probably due to this issue, has been observed at ITER when two FixedScalarArray arrays are used with differing sizes. This is with 7.0.1.1

Crashes appear in standard command line clients when structures containing bounded arrays of different sizes are involved. Both unbounded arrays and bounded arrays of the same size work do not cause crashes. The server was showing no malfunctions in any case.

@mdavidsaver
Copy link
Member Author

I've pushed 727153e which marks bounded/fixed types as deprecated through the FieldCreate and FieldBuilder interfaces. This can be reversed when/if someone demonstrates a fix. I'd also like to see some concrete use cases of how a client would consume and use the size bound.

@ldoolitt
Copy link

Hello, @mdavidsaver ! Fancy meeting you here. :-)
Is it expected to see six of these deprecation warnings when building epics-base-7.0.8?
In 2024, over five years since you deprecated them?
At least your deprecation message pointed to a useful place.

@dirk-zimoch
Copy link
Contributor

Many deprecated functions are still called in tests. Of course it makes sense to test deprecated functions too as long as they still exist. But maybe the deprecation warning can be switched off when compiling tests? (Maybe generally in EPICS base, not only in pvData.)

@ralphlange
Copy link
Contributor

I would argue against that.

Generally, deprecation warnings are good early warning signs.
We already softened the situation by explicitly not treating deprecation warnings as errors, which would break the build.

Unless we have a way to specifically mask out the deprecation warning in the exact six cases where we know we are calling deprecated functions, I would rather accept a single digit number of false positives than hide an unknown number of real issues.

@dirk-zimoch
Copy link
Contributor

Maybe then only in test code that is meant to test known deprecated functions do this to silence the warning:

#undef EPICS_DEPRECATED
#define EPICS_DEPRECATED

Test the deprecated functions last in order not to silence the warning too early.

@dirk-zimoch
Copy link
Contributor

Stupid me. One needs to do this before #includeing the header file with the deprecated functions, but after #includeing compilerSpecific.h. Thus it is more of an all-or-nothing choice.

@mdavidsaver
Copy link
Member Author

Is it expected to see six of these deprecation warnings when building epics-base-7.0.8?

Yes. My focus of development in recent years is PVXS. As PVXS matures, I see the pv*CPP modules as increasingly deprecated in their entirety.

Since no one has complained in the intervening years, I think it is reasonable to proceed with step two (see #52 (comment)) and remove these API calls. As internal APIs, the deprecation warnings could then be removed.

fyi. step 2 (decode but not encode) is currently what PVXS supports wrt. the bounded/fixed array types.

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

No branches or pull requests

4 participants