search

epics-base / pvDataCPP

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

Field sub-class comparison is incomplete #52

Open mdavidsaver opened 6 years ago

mdavidsaver commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

ralphlange commented 6 years ago

@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 commented 5 years ago

I've pushed 727153e96587597b3b3236d48644d258fd7bdb5b 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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 https://github.com/epics-base/pvDataCPP/issues/52#issuecomment-430482285) 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.