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

Preparatory work to facilitate utf8 transition #343

Merged
merged 6 commits into from
May 25, 2021

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