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.

Sign up for GitHub

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

Jump to bottom

Extend the behaviour of Utf8 to replicate that of strict Text #1386

Draft

adithyaov wants to merge 26 commits into master from utf8-text

Member

adithyaov commented Dec 23, 2021

Few undefined APIs.
Few APIs need to be improved, need to be made more performant.

This PR adds the general structure and some basic implementation.

For reviewer:

The only thing that needs to be reviewed are the XXX comments and the export list.
We might not need a few APIs. The docs are copied with minor modification, they don't really need to be reviewed.
I wanted to add individual small commits but at one point it became too late.

adithyaov force-pushed the utf8-text branch 7 times, most recently from 9c92cb8 to ee78234 Compare

December 30, 2021 19:11

adithyaov force-pushed the utf8-text branch 2 times, most recently from f9418ae to 6f008eb Compare

December 31, 2021 05:49

harendra-kumar reviewed

View reviewed changes

Member

harendra-kumar left a comment

I only reviewed the Type module. Will review others later.

Can you arrange the code in define before use order? For example the pack operation is used before it is defined. Similarly, toArray.
Operations that apply to generic array modules can be moved to the array module.

src/Streamly/Internal/Data/Array/Foreign/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Data/Array/Foreign/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8.hs Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

adithyaov force-pushed the utf8-text branch 3 times, most recently from 14efcf7 to 7a23ccd Compare

January 7, 2022 09:41

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Eliminate.hs Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Eliminate.hs Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Eliminate.hs Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Eliminate.hs Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Eliminate.hs Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Reduce.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Reduce.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Reduce.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Reduce.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Reduce.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Transform.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

harendra-kumar reviewed

View reviewed changes

src/Streamly/Internal/Unicode/Utf8/Type.hs Outdated Show resolved Hide resolved

adithyaov force-pushed the utf8-text branch 2 times, most recently from ce3fa66 to 6a5e2d2 Compare

January 18, 2022 17:23

adithyaov force-pushed the utf8-text branch from 6a5e2d2 to 534e006 Compare

January 18, 2022 17:38

adithyaov added 26 commits

February 12, 2022 08:18


          Export nil in immutable foreign array modules

2cb75c3


          Extend the behaviour of Utf8 to replicate that of strict Text

56a045a

* Few undefined APIs.
* Few APIs need to be improved, need to be made more performant.

This commit adds the general structure and some basic implementation.


          Split Unicode.Utf8 into Unicode.Type and Unicode.Transform

d8a5a76


          Break out Eliminate, Generate, and Reduce from Unicode.Utf8

bd4c051


          Remove specific exports and export entire modules instead

95fe359


          Comment out isInfixOf as it is memory intensive

d606c34


          Add an IsString and a Show instance for Utf8

25b84fc


          Implement the basic write fold for Utf8

a419c5c


          Disable unimplemented docspec examples

a5492cd


          Comment out a few APIs to help with the compilation on GHCJS

2cb9aa1


          Rewrite time complexity in a different way

119633a


          Rearrange function order in "defined before use" in Utf8.Type

4c467cc


          Rename stream and unstream to toStream and fromStream

e71127a


          Make minor changes and encode the review comments on Utf8.Type

426a52d


          Add a rewiew comment about adding general array combinators

74e1280


          Cut short the functions hiding from Prelude in Utf8.*

464a444


          Add "Unimplemented" tag to unimplemented functions in Utf8.*

9c0cde5


          Remove wrong/unecessary implementation

eb3d9f1


          Improve Utf8.partition

7ecb533


          Use Bifunctor.second & remove redundant commented code 4rm Utf8.Type

d8a0897


          Improve Utf8.singleton

9bce2f5


          Remove ambiguious time complexity specified

98e4400


          Add some comments regarding removing MonadIO in Utf8

6af34fa


          Give credit to the text package in Utf8

07c248a

Some documentation & a few examples are taken from the text package


          Perform replacement of invalid code points instead of erroring

a651995


          General cleanup and fixes

09c7288

adithyaov force-pushed the utf8-text branch from ffb792d to 09c7288 Compare

February 15, 2022 09:37

harendra-kumar requested changes

View reviewed changes

Member

harendra-kumar left a comment

Remove all unsafePerformIO. If you retain any there should be a good reason for that.
I think we should use a SerialT m a wherever we are using a [a]. That will provide a more efficient way of composing the APIs. I do not think we need a drop in replacement for text. We can add the missing APIs in a compat module if we feel that is needed.

src/Streamly/Internal/Unicode/Utf8.hs

+              {-# INLINE zip #-}
+              zip :: Utf8 -> Utf8 -> [(Char,Char)]
+              zip a b =
+                  unsafePerformIO

Member

harendra-kumar Feb 15, 2022

Use identity monad instead of unsafePerformIO.

src/Streamly/Internal/Unicode/Utf8.hs

+                  case Prelude.filter (not . null) ts of
+                      [] -> empty
+                      [t] -> t
+                      xs -> Prelude.foldl1 append xs

Member

harendra-kumar Feb 15, 2022

Should use something like this:

streamly/src/Streamly/Internal/Data/Array/Stream/Foreign.hs

Line 131 in 3cba8fa

concat m = fromStreamD $ D.unfoldMany A.read (toStreamD m)

src/Streamly/Internal/Unicode/Utf8.hs

+              -- /Time complexity:/ O(n)
+              {-# INLINE concatMap #-}
+              concatMap :: (Char -> Utf8) -> Utf8 -> Utf8
+              concatMap f = concat . foldr ((:) . f) []

Member

harendra-kumar Feb 15, 2022

I would prefer to use stream based implementation instead of using list.

src/Streamly/Internal/Unicode/Utf8.hs

+              -- /Time complexity:/ O(n)
+              {-# INLINE unlines #-}
+              unlines :: [Utf8] -> Utf8
+              unlines = concat . List.map (`snoc` '\n')

Member

harendra-kumar Feb 15, 2022

using snoc may not be most efficient, we should implement it in a streaming way, see unlines in Unicode.Stream module.

src/Streamly/Internal/Unicode/Utf8/Type.hs

+              -- XXX This would require rewriting Array.Foreign.write-ish functions. We can't
+              -- use the underlying Array.Foreign.Mut.write-ish
+              {-# INLINE fromStream #-}
+              fromStream :: SerialT IO Char -> Utf8

Member

harendra-kumar Feb 15, 2022

This must be fromStream :: SerialT m Char -> m Utf8. You cannot have the result pure when the stream is not pure.

src/Streamly/Internal/Unicode/Utf8/Type.hs

+              -- /Time complexity:/ O(n)
+              {-# INLINE_NORMAL unpack #-}
+              unpack :: Utf8 -> String
+              unpack = unsafePerformIO . Stream.toList . toStream

Member

harendra-kumar Feb 15, 2022

Use identity monad, do not use unsafePerformIO anywhere.

harendra-kumar marked this pull request as draft

July 5, 2023 17:24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment