proto3-wire icon indicating copy to clipboard operation
proto3-wire copied to clipboard

Fix parsedField comment or code

Open ndmitchell opened this issue 6 years ago • 3 comments

Looking at https://github.com/awakesecurity/proto3-wire/blob/43d8220dbc64ef7cc7681887741833a47b61070f/src/Proto3/Wire/Decode.hs#L288-L293

It says "To comply with the protobuf spec, if there are multiple fields with the same field number, this will always return the last one" and then returns the first one. That seems to be an optimisation from @gbaz. Was that intentional? If so, perhaps update the comment? If not, perhaps update the code? Our optimised version used lastMay there.

ndmitchell avatar Apr 16 '19 17:04 ndmitchell

If I recall, we keep the list in reversed order. As we approach Easter, it is appropriate that in this code, the last shall be first.

gbaz avatar Apr 16 '19 20:04 gbaz

If that''s the case, we should document it on the RawField type, and update the comment to parsedField. Might be worth adding a test though, since changing it to be the last doesn't seem to break anything for me.

ndmitchell avatar Apr 16 '19 20:04 ndmitchell

It was also suggested we move to a newtype, which would be even clearer in this regard...

gbaz avatar Apr 16 '19 20:04 gbaz