Skip to content
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

Multiple PRs #2662

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Multiple PRs #2662

merged 2 commits into from
Feb 22, 2024

Conversation

adithyaov
Copy link
Member

No description provided.

@adithyaov adithyaov force-pushed the rm-imm-cloning branch 2 times, most recently from b8b6bfa to 1875e5d Compare January 14, 2024 19:14
@adithyaov adithyaov changed the title Add documentation to Array.clone and Array.pinnedClone Multiple PRs Jan 14, 2024
@adithyaov adithyaov force-pushed the rm-imm-cloning branch 3 times, most recently from 371720b to 89bcc28 Compare January 16, 2024 15:48
@adithyaov adithyaov force-pushed the rm-imm-cloning branch 2 times, most recently from d15e389 to 1f6ac70 Compare February 8, 2024 22:28
in Skip (MECSBuffer remElem st1)
Yield arr st1 ->
let len = Array.length arr
in if (len .&. 1) == 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use odd instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odd uses div

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm..that's odd.

in Yield w16
(MECSYieldAndBuffer arr1 lstElem st1)
Skip s -> Skip (MECSBuffer remElem s)
Stop -> Stop -- Here the last Word8 is lost
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of strict decoding, this should be an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has nothing to do with utf16 decoding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in the benchmark for now anyway, so I don't want to spend more time on this. We can change this if this use-case ever arises.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it has nothing to do with it then we should not use it in decoding :-) Because decoding should fail if it finds the input is truncated. If this drops silently, decoder won't even know that some part of the input was dropped, it will just happily use whatever it got.

@adithyaov adithyaov force-pushed the rm-imm-cloning branch 2 times, most recently from f5e9741 to 1e7224a Compare February 20, 2024 14:09
Comment on lines 33 to 34
, decodeUtf16'
, decodeUtf16
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep these unexposed. Don't know if we'll ever use this without taking byte-order into consideration.

- Add identity tests
- Add benchmarks
- Add Word16 to Word8 helpers
copyStreamUtf16 :: Handle -> Handle -> IO ()
copyStreamUtf16 inh outh =
Stream.fold (Handle.writeChunks outh)
$ fmap Array.castUnsafe $ Array.chunksOf (arrayPayloadSize (16 * 1024))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use defaultChunkSize instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the number of elements constant for comparison.

@adithyaov adithyaov merged commit 86e8acc into master Feb 22, 2024
12 of 16 checks passed
@adithyaov adithyaov deleted the rm-imm-cloning branch October 15, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants