search

bscarlet / llvm-general

Rich LLVM bindings for Haskell (with transfer of LLVM IR to and from C++, detailed compilation pass control, etc.)
http://hackage.haskell.org/package/llvm-general
132 stars 38 forks source link

LLVM 3.9 #180

Closed cocreature closed 8 years ago

cocreature commented 8 years ago

LLVM 3.9 is supposed to be out next week, I created this using rc1. I still need to cleanup some stuff (which is why it is marked WIP) but all tests are passing.

I haven’t yet figured out why travis is failing, maybe the version in the repo is not up2date. Also this should go in a new branch but I have no idea how I can make a pullrequest that is supposed to create a new branch.

While it is probably best to wait to merge this until 3.9 is released, I wanted to create this PR now to avoid duplicating work.

bscarlet commented 8 years ago

I've created an llvm-3.9 branch off of llvm-3.8. Apparently github has a new feature to allow changing the base branch of a PR:

https://github.com/blog/2224-change-the-base-branch-of-a-pull-request

but for some reason my new llvm-3.9 branch doesn't show up in the list when I try that for your PR.

cocreature commented 8 years ago

I think I addressed all your comments. Travis is still failing, haven’t yet figured out why that is. Also I’m still not able to select llvm-3.9 as a base branch (I was hoping for some cache invalidation issue). Weirdly enough on https://github.com/bscarlet/llvm-general/tree/llvm-3.9 there is a link to this PR. It would be easiest if you just merge manually once you think it’s ready but I’m fine with opening a new PR if you prefer that.

bscarlet commented 8 years ago

I think I've now commented on everything I think needs changes. Sorry my comments get stretched out over time. I think this sort of process would go better in future with more, smaller PRs so each can get committed along the way (and so I don't have to re-review so much).

cocreature commented 8 years ago

I think this sort of process would go better in future with more, smaller PRs so each can get committed along the way (and so I don't have to re-review so much).

Comitting some intermediate state that doesn’t even compile or fails 90% of the tests is not particularly useful.

bscarlet commented 8 years ago

I disagree. The evolution of the code from the initial sad state you describe to all-green is a fact, whether hidden or not. By exposing the steps to get there in more commits & PRs; it's easier to discuss individual changes; it's easier to build consensus on the principles of the codebase code earlier, so later changes involving the same principles can proceed with less friction; and we gain a ratchet - each piece I accept done with, I don't need to re-review it as you make other changes, nor are you in any doubt about it as you make further changes.

cocreature commented 8 years ago

I think I addressed all your comments.

bscarlet commented 8 years ago

Just style nits left. After this, looks good. I see you've changed the base branch - github support let me know they've fixed the bug that kept llvm-3.9 from showing up.

cocreature commented 8 years ago

Just style nits left. After this, looks good.

Show all be fixed now

I see you've changed the base branch - github support let me know they've fixed the bug that kept llvm-3.9 from showing up.

Yeah I contacted github support as well :)

bscarlet commented 8 years ago

See issue #185 for some details of the travis failure.