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

Add support for padding and char replication #14

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

wismill
Copy link
Contributor

@wismill wismill commented Sep 4, 2023

  • Add Char replication functions prependChars and appendChars
  • Add padding functions justifyLeft, justifyRight and center.
  • Move low-level MArray manipulation to their own module Data.Text.Builder.Linear.Array.
  • Add corresponding tests and benchmarks.

See also #13, which propose a similar interface for padding.

Note that it adds a dependency on linear-base to use move and Ur.

@wismill wismill marked this pull request as ready for review September 4, 2023 09:34
@wismill
Copy link
Contributor Author

wismill commented Sep 5, 2023

Rebased.

-- "xxxxAAAxxx"
-- >>> runBuffer (\b -> center 5 'x' (appendChars 6 'A' b))
-- "AAAAAA"
center ∷ Word → Char → Buffer ⊸ Buffer
Copy link
Owner

Choose a reason for hiding this comment

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

Just curious, do you have a use case for such functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the padding functions specifically? Mainly pretty-printing. For example, I generate some C files with lots of arrays, but I want them to be well presented to facilitate review, so padding with 0 for hex values and spaces for general text comes quite handy.

This also bring the API closer to the text one.

Copy link
Owner

Choose a reason for hiding this comment

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

Could you possibly share a snippet using the proposed API? What happens when you already have some data in a Buffer and want to append a padded hexadecimal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After try using the API with non-trivial examples, I see what you mean 😄

The immediate solution I came with is to add the following function to the API:

newEmptyBuffer  Buffer  (# Buffer, Buffer #)
newEmptyBuffer (Buffer t@(Text arr _ _)) =
  (# Buffer t, Buffer (if isPinned arr then memptyPinned else mempty) #)

then an example of the use of justifyRight is:

-- | Convenient function to append padded hex
infixl 6 |>&&
(|>&&) :: (Integral a, FiniteBits a) => Buffer  (Word, a) -> Buffer
b |>&& (w, n) = case newEmptyBuffer b of
  (# b', empty #) -> (b' |># "0x"#) >< justifyRight w '0' (empty |>& n)

-- Example
runBuffer \b -> (b |># "Foo "#) |>&& (4, 0xf :: Word) |># "; "# |>&& (4, 0xabc :: Word)
-- "Foo 0x000f; 0x0abc"

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, let's add newEmptyBuffer than. Not sure about the name though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. About naming: I did not named simply emptyBuffer because it requires an input. But I have not strong opinion about this. Other candidates: makeEmptyBuffer or the shorter mkEmptyBuffer.

@wismill
Copy link
Contributor Author

wismill commented Sep 8, 2023

@Bodigrim Rebased, with the dependency to linear-base removed by adapting the tiny part we need. I left their copyright, but the code is so tiny that I wonder if we could just apply our default copyright.

@Bodigrim
Copy link
Owner

Bodigrim commented Sep 9, 2023

@Bodigrim Rebased, with the dependency to linear-base removed by adapting the tiny part we need. I left their copyright, but the code is so tiny that I wonder if we could just apply our default copyright.

I think all we need is

fooInt :: (Int  a)  (Int  a)
fooInt = unsafeCoerce

fooWord :: (Word  a)  (Word  a)
fooWord = unsafeCoerce

There is nothing unsafe about them, so I'd suggest just define them in the module where they are used.

@wismill
Copy link
Contributor Author

wismill commented Sep 10, 2023

There is nothing unsafe about them, so I'd suggest just define them in the module where they are used.

@Bodigrim done.

@Bodigrim
Copy link
Owner

There is still a conflict in test/Main.hs?..

- Add `Char` replication functions `prependChars` and `appendChars`
- Add padding functions `justifyLeft`, `justifyRight` and `center`.
- Move low-level `MArray` manipulation to their own module
  `Data.Text.Builder.Linear.Array`.
- Add corresponding tests and benchmarks.
Some situations may require a fresh empty buffer. For example, if one
want to define a function that append padded hexadecimal numbers, the
use of a mere `justifyRight` requires the input `Buffer` to be empty.

We introduce `newEmptyBuffer` to solve this issue. It has the caveat to
require an existing `Buffer`, because we need to to know if the array is
pinned or not.

Working example for padded hexadecimal numbers:

    -- | Convenient function to append padded hex
    infixl 6 |>&&
    (|>&&) ∷ (Integral a, FiniteBits a) ⇒ Buffer ⊸ (Word, a) → Buffer
    b |>&& (w, n) = case newEmptyBuffer b of
    (# b', empty #) → (b' |># "0x"#) >< justifyRight w '0' (empty |>& n)

    -- Example
    runBuffer \b → (b |># "Foo "#) |>&& (4, 0xf ∷ Word) |># "; "# |>&& (4, 0xabc ∷ Word)
    -- "Foo 0x000f; 0x0abc"
@wismill
Copy link
Contributor Author

wismill commented Sep 10, 2023

@Bodigrim Sorry for that, I did not see it. Fixed by rebasing.

Also, congrats for the release of tasty-1.5 ! I really like the progress info and the improvement of -p .

src/Data/Text/Builder/Linear/Array.hs Show resolved Hide resolved
prependExact
totalLen
(\dst dstOff → unsafeWrite dst dstOff ch *> unsafeTile dst dstOff totalLen cLen)
buff
Copy link
Owner

Choose a reason for hiding this comment

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

Let's reduce code duplication here: in both cases you can run prependExact (utf8Length ch * fromIntegral count) fun buff, it's only fun which depends on isAscii ch.

utf8Length is a fast function, there is little to save by not calling it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/Data/Text/Builder/Linear/Char.hs Show resolved Hide resolved
-- >>> runBuffer (\b -> justifyRight 10 'x' (appendChars 3 'A' b))
-- "xxxxxxxAAA"
-- >>> runBuffer (\b -> justifyRight 5 'x' (appendChars 6 'A' b))
-- "AAAAAA"
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a usage example with newEmptyBuffer, it's untrivial to come up with for a new user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

-- The first 'Buffer' is the input and the second is a new empty 'Buffer'.
--
-- Note: a previous buffer is necessary in order to create an empty buffer with
-- the same characteristics.
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a usage example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Bodigrim Bodigrim merged commit db70163 into Bodigrim:master Sep 11, 2023
6 checks passed
@Bodigrim
Copy link
Owner

Thanks, great!

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