libmir / mir-ion

Ion, JSON, YAML, CSV, CBOR and Msgpack serialization framework
Apache License 2.0
14 stars 6 forks source link

Refactor MessagePack serialization backend to remove msgpack-d dependency #4

Closed hatf0 closed 2 years ago

hatf0 commented 2 years ago

This PR will fully remove the dependency on msgpack-d for the serialization backend. All features should be implemented (and are verified to be correct with the upstream MessagePack spec).

Let me know if this needs any more work! Thanks.

codecov-commenter commented 2 years ago

Codecov Report

Merging #4 (87c352e) into master (a1a0abe) will increase coverage by 0.56%. The diff coverage is 99.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
+ Coverage   84.88%   85.45%   +0.56%     
==========================================
  Files          31       32       +1     
  Lines        6624     6902     +278     
==========================================
+ Hits         5623     5898     +275     
- Misses       1001     1004       +3     
Impacted Files Coverage Δ
source/mir/ion/ser/msgpack.d 98.91% <99.15%> (ø)
source/mir/ion/deser/json.d 84.21% <0.00%> (ø)
source/mir/ion/ser/package.d 96.72% <0.00%> (+0.01%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a1a0abe...87c352e. Read the comment docs.

hatf0 commented 2 years ago

Do not merge yet -- I have to refactor some bits and pull out the usage of ScopedBuffer (as it's now going out of scope and causing a use after free)

9il commented 2 years ago

Please add extra 4 spaces for the whole MsgpackSerializer. And use the same order of methods like in the original code. That is needed to reduce the diff for easier review.

9il commented 2 years ago

We need 100% coverage of the new code for the message pack. It is a critical part of code used in production.

hatf0 commented 2 years ago

We need 100% coverage of the new code for the message pack. It is a critical part of code used in production.

@9il, I was able to get coverage up to 98% on this file, if that's fine. The only code that isn't covered is: https://github.com/libmir/mir-ion/blob/f143764fc10701beda287f72cc38eec0042f5b8a/source/mir/ion/ser/msgpack.d#L572-L575 https://github.com/libmir/mir-ion/blob/f143764fc10701beda287f72cc38eec0042f5b8a/source/mir/ion/ser/msgpack.d#L537-L540 Both of these aren't feasible to test (as they'd require that we allocate a const(char)[] array that is 4.29 GB in size) https://github.com/libmir/mir-ion/blob/f143764fc10701beda287f72cc38eec0042f5b8a/source/mir/ion/ser/msgpack.d#L317-L318 and I'm not sure how to trigger this code path.

9il commented 2 years ago

@9il, I was able to get coverage up to 98% on this file, if that's fine. The only code that isn't covered is:

That is enough, thank you

hatf0 commented 2 years ago

Alright. I've added your requested fixes -- let me know if there's anything else that need fixing.

9il commented 2 years ago

Looks good.