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 intances of the Storable class. #15

Merged
merged 3 commits into from
Mar 11, 2015

Conversation

rabipelais
Copy link
Contributor

This addresses issue #14.
I am not very experienced with TH, but this compiles and seems to do the Right Thing when I tested it on a side project.
Do you have any guidelines to contributing, regarding for example code style?

Generate the functions `sizeOf` with the sum of the sizes of the
elements, `alignment` with the maximum alignment, and peek and poke.
Note that the commit also removes some trailing whitespaces.
@nikita-volkov
Copy link
Owner

Thanks for your contribution.

Unfortunately, as Travis indicates, your PR fails to compile with GHC 7.8 and 7.10. So I can't merge it until the issue is fixed.

@rabipelais
Copy link
Contributor Author

Is there a way to view a detailed error message? Because I suspect the issue is a timeout when compiling.

@nikita-volkov
Copy link
Owner

You may be right, since this project requires long compilation times, however 10 minutes seems just too long. Usually it is an indication of an infinite loop during compilation, and since we're dealing with compile-time macros here, chances are, it is the case.

@rabipelais
Copy link
Contributor Author

The log for 7.8 says it compiles correctly, but timesout on the Test suite doctest: RUNNING..., could this be because of my changes? (I didn't touch the doctest Main.hs nor any doctest comments.)
It does pass the other 7.8 build, the one without lower_bound_dependencies=1, although both compile correctly. I find this all really weird.

As for the 7.10 build, they made some changes to the TH API, and I am working on it.

@nikita-volkov
Copy link
Owner

Oh. You're right.

It actually failed on both executions on 7.8 the first time, but then I relaunched the build and one of them just passed fine. I've relaunched the other one again just now - we'll see how it'll go. Probably there is no issue with 7.8 compatibility and it merely takes doctest too long to run.

@nikita-volkov
Copy link
Owner

I've updated the Travis configuration to extend the 10 minute timeout. We'll see whether the commit aa594e5 fixes the issues for 7.8.

@rabipelais
Copy link
Contributor Author

Thank you! More time seemed to solve the issue! I still find weird that the new build, with the extra time, took less time than the previous one, and would therefore not need the extra time....

I will try to solve the issue with 7.10

For TH >= 2.10.0 instance contexts are simple types. This change applies
to the instance for `Storable`.
@rabipelais
Copy link
Contributor Author

It should work now (compiles locally), but for 7.10 Travis complains with the error cabal: Prelude.chr: bad argument as referenced in this cabal bug. I see you commented there, how did you manage to solve it?

(Also, I don't know if this pull request is taking into account the commit aa594e5 where you solved the timeout issue. What would be proper here? For me to merge pr/15 into this pull request? However, note that the Travis builds passed.)

@nikita-volkov nikita-volkov merged commit 1f67de6 into nikita-volkov:master Mar 11, 2015
@nikita-volkov
Copy link
Owner

It's merged and released now as 0.2.2. Thanks again for your contribution and for being responsible.

Travis complains with the error cabal: Prelude.chr: bad argument as referenced in this cabal bug. I see you commented there, how did you manage to solve it?

I didn't manage to solve it. That bug is the sole reason why failures are allowed for the 7.10 build target.

@nikita-volkov
Copy link
Owner

Oops. I forgot that I had some unreleased API-changing updates on master. So a proper version for this is 0.3.0.

@rabipelais rabipelais deleted the storable branch March 11, 2015 06:25
@rabipelais
Copy link
Contributor Author

Thanks for merging! And for the hard work you put on this library. Shame about the cabal bug.

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