markcmiller86
commented
9 months ago Ok, I've looked at this a bit more closely.
First, htri_t return value applies only to the can_apply() method, not the set_local() method.
Next, in H5Z-ZFP's implementation of can_apply() you can see the 4th argument passed to H5Z_ZFP_PUSH_AND_GOTO is either a -1 (fail) or 0 (false). The fail cases are all the cases where we're looking up HDF5 resources. If any of those calls fail, then we're right to fail can_apply() too. After we've obtained all the HDF5 resources we need, all the other checks return 0 (false) if they don't pass which means the filter will get silently skipped. For example, an attempt to apply it to character data will silently skip it.
So, its mostly right. I say mostly because there is one check we perform, checking whether ZFP library is configured with 8 bit streams or not. We currently fail that (e.g. return -1). I think we wanted that check in the user's face so that there would be no missing it. And, I feel like this is correct behavior because in a situation where a user that wants to use ZFP compression but cannot because they don't have ZFP configured correctly (and not because the data they are trying to apply it to is simply not suitable for ZFP compression), that represents a serious error.
So, I think currently implemented behavior in this regard is correct.
See this comment. Long story short, its not clear we're handling the return value correctly for these cases. We maybe erroring when we should simply be returning false.