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 compatibility to GHC 7.8.x #45

Merged
merged 1 commit into from
Oct 1, 2015
Merged

Add compatibility to GHC 7.8.x #45

merged 1 commit into from
Oct 1, 2015

Conversation

bkaestner
Copy link
Contributor

This PR basically introduces a bunch of CPP #ifs to import additional operators from Control.Applicative (in most cases) or Data.Foldable (in one case). See #42 for the original issue.

I didn't change the existing import Control.Applicative lines, since they would lead to redundant import warnings on GHC 7.10.x, and instead put the #if MIN_VERSION makros at the end of all imports in all files. in all files that used only <$> in one place, I replaced <$> by infix fmap to keep the number of changes per file a little bit smaller.

While I was at it, I also removed the "recent version of base" comment in the readme, since it works now with an older one (by the way, base-4.6 compatibility seems possible, currently experimenting).

Things left to do:

  • change Travis configuration to automatically check compatibility with 4.7.x

Remarks

  • the fixity declaration of /=\ in tests/Utils.hs lead to some issues. I've added a comment, which will prevent backslash-before-newline trouble.
  • while the current version works, it introduces many changes to many files. The next version of base shouldn't break anything (at least regarding the Control.Applicative part), but one should probably think of a less error prone strategy, like a custom prelude (so that there's only a single point of CPP).

@mrkkrp
Copy link
Owner

mrkkrp commented Sep 30, 2015

Great work, thank you. I'm a bit sleepy right now, so I will return to this tomorrow to check and merge. While you're at it, please amend that last commit and also edit “(< 7.10)” → “(< 7.8)” (that's in the section “Megaparsec vs Parsec”) changing description of the commit accordingly.

Yep, we'll need to make Travis test GHC 7.8 too, but it's trivial. I'm not sure whether we should use custom Prelude and real benefits of such solution. I don't find conditional imports of Control.Applicative particularly ugly (in any sense) right now.

This patch introduces compatibility to base-4.7.0.x. It was tested
on Win 8.1 x86_64, using GHC 7.8.4. It mainly consists of a bunch
of #if !MIN_VERSION(4,8,0) ... #endif additions and a lower bound
on base in the cabal file as well as a general introduction of the
CPP extension via default-extensions.

It also removes a potential error source in tests/Util.hs, since
the backslash in /=\ can lead to strange quirks on certain systems
(backslash and newline only separated by whitespace).

Other, squashed commits:

- Remove 'recent version of base' from Readme

- Change necessary version of GHC
@bkaestner
Copy link
Contributor Author

I don't find conditional imports of Control.Applicative particularly ugly (in any sense) right now.

Yeah, the conditional imports aren't that bad. However, the conditional imports/redefines on base-4.6.x.x... Well, see #46 :D.

While you're at it, please amend that last commit

Err, I think I've misread your comment as git rebase master --interactive and squashed both commits into one. Not sure whether you wanted that or only the 7.10 -> 7.8 amended. Well, at least it keeps the history clean.

@mrkkrp
Copy link
Owner

mrkkrp commented Sep 30, 2015

Yes, I meant only that last commit dealing with README.md file. But in its current state it's also fine. I'm not sure compatibility with base-4.6.x.x is a priority, however. We will talk about this tomorrow in dedicated thread.

mrkkrp added a commit that referenced this pull request Oct 1, 2015
Add compatibility to GHC 7.8.x
@mrkkrp mrkkrp merged commit 4c5c3d8 into mrkkrp:master Oct 1, 2015
tomjaguarpaw pushed a commit to tomjaguarpaw/megaparsec that referenced this pull request Sep 29, 2022
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.

None yet

2 participants