Metadata 3.8 #168
Metadata 3.8 #168
Conversation
| | MetadataStringOperand String | ||
| | MetadataNodeOperand MetadataNode | ||
| -- MetadataStringOperand String | ||
| -- MetadataNodeOperand MetadataNode |
There was a problem hiding this comment.
Why have you commented these out? The 3.8 language spec still says that "Metadata can be used as function arguments".
There was a problem hiding this comment.
Good catch, I forgot this. I would prefer to have an an MetadataOperand Metadata constructor since this is closer to the representation in LLVM. Otherwise we would need to do some normalization to remove nesting of ValueAsMetadata and MetadataAsValue. I’m pretty busy right now so I don’t know when I’ll get to it, but hopefully sometime next week.
|
Sorry that it took so long I simply forgot that this PR still needed changes. I’ve now added support for MetadataAsValue which is the new way of passing metadata as arguments. |
|
Can you try to get the travis build working for the 3.8 branch? I'll try to get my own 3.8 environment set up so I can check myself of course, but if travis is working I can accept PRs just by looking over the code and pushing a button. |
|
Also my travis configuration uses the travis container config which is a bit faster and uses |
|
Using the travis container stuff is great. Please put it in a separate PR so you or I can more easily try it for other branches. In general, more, smaller, individually verifiable PRs would be highly preferable to fewer big ones. |
| | MetadataNodeReference MetadataNodeID | ||
| deriving (Eq, Ord, Read, Show, Typeable, Data) | ||
|
|
||
| data Metadata |
There was a problem hiding this comment.
I've tried to keep the haddock coverage of the exposed llvm-general API at 100%.
Please you give this type a haddock comment, ideally with linking into the llvm.org hosted language reference and/or source designed to clarify to the uninitiated user how the various llvm-general metadata types map to the non-haskell LLVM world. Often just a link is sufficient. For cases where the LLVM docs are themselves confusing, more is sometimes justified.
|
Your comments should be fixed now. |
|
While I’m completely fine with that because I don’t particularly care about hackage releases, I’m not sure this makes sense. Once we support the feature set we supported with older llvm versions (not saying that this PR necessarily achieves it, but it’s at least a different thing than supporting the complete langref) a release seems just appropriate as it only improves the current situation.
#177 gives us working builds (at least for 7.10 and 8.0, 7.8 is broken for unrelated reasons). To get the tests to pass it is necessary to merge this PR so waiting until they are fixed elsewhere won’t work. |
|
Having watched LLVM evolve for a while, I think it's important that llvm-general releases continue to track the actual functionality of LLVM, rather than just managing to build against it. The point of llvm-general-3.8 isn't just to support a fixed llvm-general API against a moving LLVM implementation, but rather to present in Haskell the LLVM 3.8 API as a opposed to that of previous versions. |
|
Just to make sure this is not blocked on my side, is there anything else you want me to do here before this can be merged? |
|
Right now it's blocked on me. (I'm currently learning more about travis-ci's handling of pull requests, as I gather your changes that have been merged onto llvm-3.8 ought to make this PR build and perhaps even pass. I have some to learn about the mapping of github & travis-ci's UIs to the underlying stuff with which I'm familiar in git). |
|
You might want to try simply restarting the build. Not sure if this will work if the upstream branch changed but it’s worth a try. |
|
Alternatively I could rebase this if you prefer that. |
|
I did rerun the build, and it failed as it did before the new additions on llvm-3.8. I presume the "merge pull request" button would do the same thing as the github-provided "command line instructions", but I don't know off the top of my head what the default merge strategy would do (revert the intervening changes from the other PRs or not). Either I can try it so I (we?) learn - at the potential cost of having to revert it, or you can rebase. I'm okay with either - please let me know which you prefer. |
cocreature
force-pushed
the
metadata-3.8
branch
from
193fa14
to
d37fd41
Compare
|
I just rebased and 7.10 and 8.0 are now green, 7.8 is still failing for what I think is an unrelated change. If you want I can also put the fix in this PR, but since you prefer small PRs it seems to make more sense to merge this one and then fix it in a separate PR. |
|
Yes. Passing on 7.10 and 8.0 is sufficient evidence for me. |
Since the rest of the big PR is merged, this is a smaller PR with the remaining metadata changes (and a fix from @xldenis) to make it easier to see what’s going on.
It mostly works but there are two problems: I’m not sure about the decoding of
MDNodesince theisFunctionLocalattribute no longer exists. One of the tests doesn’t work because I still haven’t managed to figure out if this is simply no longer possible or if it just uses some very weird syntax.