[pbs-devel] [PATCH v3 pxar 06/58] encoder: move to stack based state tracking

[Christian Ebner]

On 4/4/24 10:48, Fabian Grünbichler wrote:
> On April 3, 2024 1:01 pm, Christian Ebner wrote:
>> On 4/3/24 11:54, Fabian Grünbichler wrote:
>>>
>>> should we still have some sort of checks here? e.g., when dropping an
>>> encoder, how should self.finished and self.state look like? IIUC, then a
>>> dropped encoder should have an empty state and be finished (i.e.,
>>> `close()` has been called on it).
>>>
>>> or is this simply not relevant anymore because we only create one and
>>> then drop it at the end (but should we then have a similar mechanism for
>>> EncoderState?)
>>
>> The encoder should now be consumed with the `close` call, which takes
>> ownership of the encoder and drops it afterwards, so all the state
>> checks should happen there.
>>
>> Previously, the encoder finish consumed the per-directory level encoder
>> object, passing possible errors up to the parent implementation, which
>> is not possible now since there is only one encoder instance. I did not
>> want to panic here as the checks should be done in the close now, so the
>> Drop implementation was removed.
> 
> but now the equivalent is the EncoderState (which is per directory).
> 
>> Not sure what to check in a Drop implementation the EncoderState. What
>> did you have in mind for that? Note that errors get propagated to the
>> parent state in the encoder finish calls now.
> 
> well, basically that it is finished itself? i.e., in `finish` set a
> flag, and in the Drop handler check that it is set. right now this is
> the only place we `pop` the state from the state stack anyway, so it
> should be okay, but who knows what future refactors bring.


Okay, so will take any encoding errors stored in the state when 
finishing the directory in the `finish` calls and set a finished flag, 
and add a check in the `EncoderState`s `Drop` implementation if all the 
errors have been correctly passed along to the parent state (none 
present anymore) and the finished flag is set. Requires however to panic 
if that is not the case.