-
Notifications
You must be signed in to change notification settings - Fork 109
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
Fix the handling of packed repeated fields (#38). #64
Conversation
judah
commented
Dec 28, 2016
- Don't emit empty packed fields during encoding.
- Allow parsing packed fields as unpacked and vice versa.
- Proto3 repeated fields are packed by default.
- Don't emit empty packed fields during encoding. - Allow parsing packed fields as unpacked and vice versa. - Proto3 repeated fields are packed by default.
getSimpleVal = do | ||
Equal <- equalWireTypes name fieldWt wt | ||
get val | ||
-- Get a block of packed values, reversed. | ||
getPackedVals :: w ~ B.ByteString => Either String [a] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be simpler as "getPackedVals :: B.ByteString -> Either String [a]" and then the type equality can be implicit at the call site (in the RepeatedField case below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out GHC can infer these types (maybe due to the refactor you suggested below), so I just removed the type signature.
-- depending on how it was encoded. | ||
-- Note that if fieldWt is Lengthy (e.g., "string" or | ||
-- message) we should always parse it as unpacked. | ||
| Right Equal <- equalWireTypes name wt fieldWt -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively (ha), "RepeatedField _ f -> getPackedVals <|> getUnpackedVals <|> fail ..." and keep the "equalWireTypes" calls in the helper functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This fixed a regression of about 10% in some of the decoding benchmarks.