simd-lite / simd-json-derive

high performance Serialize and Deserialize derives
Apache License 2.0
30 stars 7 forks source link

fixed memory bugs, and all arrays of arbitrary length are supported now as well as some minor bug fixes #54

Closed Vrtgs closed 8 months ago

Vrtgs commented 10 months ago

Sorry for my shameless dependency plug, btw

Vrtgs commented 10 months ago

Thanks for the code review, I just don't know how we proceed now

Vrtgs commented 10 months ago

I quite like the changes, nice QOL improvements 👍 .

In general, I would prefer the Deserializer/Serializer to live in the heap-array crate since there are more data structure crates then there are encoding crates but since the work is done here already I don't mind merging it. :)

It's fine I moved the Deserializer/Serializer to live in the heap-array

Licenser commented 9 months ago

Heya the tests and formating isn't passing yet and would need some fixing before this can be merged, the format is easy just runign cargo format should do the trick :)

Licenser commented 9 months ago

Heya @NightMare-Vortex I updated it to resolve conflicts for the 0.11 release, it'll still need a cargo fmt run before it can be merged

Licenser commented 8 months ago

Heya @NightMare-Vortex are you still interested in finishing this PR?

Vrtgs commented 8 months ago

Heya @NightMare-Vortex are you still interested in finishing this PR?

oh yes I am, sorry I didn't see anything on my email 😅

Licenser commented 8 months ago

No worries, I know how easy it is to get flooded with mails :D that's why I figured I'll re-ping every now and then

codecov[bot] commented 8 months ago

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (b23569d) 74.85% compared to head (9704530) 75.94%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #54 +/- ## ========================================== + Coverage 74.85% 75.94% +1.09% ========================================== Files 14 14 Lines 1388 1422 +34 ========================================== + Hits 1039 1080 +41 + Misses 349 342 -7 ``` | [Files](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite) | Coverage Δ | | |---|---|---| | [src/impls.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzLnJz) | `87.50% <ø> (+13.81%)` | :arrow_up: | | [src/impls/chrono.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzL2Nocm9uby5ycw==) | `0.00% <0.00%> (ø)` | | | [src/impls/primitives.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzL3ByaW1pdGl2ZXMucnM=) | `44.30% <75.00%> (-2.04%)` | :arrow_down: | | [src/lib.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2xpYi5ycw==) | `61.90% <75.00%> (ø)` | | | [src/impls/simdjson.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzL3NpbWRqc29uLnJz) | `46.51% <50.00%> (+0.08%)` | :arrow_up: | | [src/impls/string.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzL3N0cmluZy5ycw==) | `70.00% <0.00%> (-17.50%)` | :arrow_down: | | [src/impls/array.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzL2FycmF5LnJz) | `85.14% <82.71%> (-4.44%)` | :arrow_down: | | [src/impls/collections.rs](https://app.codecov.io/gh/simd-lite/simd-json-derive/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simd-lite#diff-c3JjL2ltcGxzL2NvbGxlY3Rpb25zLnJz) | `37.01% <15.00%> (+3.34%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Licenser commented 8 months ago

As there were a bunch of conflicts I cherry picked the changes and merged them in #72

Vrtgs commented 8 months ago

Alright thanks

Vrtgs commented 8 months ago

although I really do think there should be a discussion about the safety of the parsing code, why is parse fallible, and why we parse the array, without having the values of the array in the first place