Skip to content

Conversation

@Bodigrim
Copy link
Contributor

The main work on utf8 transition is likely to be a pretty long-lived branch, but there are several preparatory changes, which improve development experience in general, and I'd like to merge them early, so that I do not spend time rebasing them.

When merging, please rebase, do not squash. Each commit is a separate, orthogonal change here, but raising 6 PRs seems an overkill.

@Bodigrim Bodigrim requested a review from Lysxia May 23, 2021 14:59
@Bodigrim
Copy link
Contributor Author

Discussion points:

  1. One might argue that it's preferable to collect all integer conversions in a single module instead of scattering them around. I think there is not much overlap between modules at the moment to warrant such move, but I would not mind to do so, if others feel it as a better alternative.

  2. Instead of repeating #ifdef ASSERT HasCallStack => #endif Bla -> Bla -> Bla everywhere, we can define in a separate module type HasCallStack' = #ifdef ASSERT HasCallStack #else () #endif and then just use HasCallStack' => Bla -> Bla -> Bla. Does it sound appealing to do so?

Copy link
Contributor

@Lysxia Lysxia left a comment

Choose a reason for hiding this comment

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

LGTM

@Lysxia Lysxia linked an issue May 23, 2021 that may be closed by this pull request
@Lysxia
Copy link
Contributor

Lysxia commented May 24, 2021

Re: discussion points. I think the way you did things is fine. In particular for 2, refactoring into a type synonym unfortunately makes it visible to haddock.

@Lysxia Lysxia merged commit e24031e into haskell:master May 25, 2021
@Bodigrim Bodigrim deleted the omnibus branch August 7, 2021 19:02
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.

Remove src/Data/Text/Internal/Unsafe/Shift.hs

2 participants