Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
60,060 ms | x64 debian > MLIR.Examples/standalone::test.toy |
Event Timeline
Hi, I am a first-time contributor to LLVM. I just requested a review from the last two people to make commits to the OCaml LLVM bindings, because I wasn't sure who the code owner was. For context, see https://discourse.llvm.org/t/guidance-for-doing-my-first-contribution/65590/3. Please let me know if I'm doing anything wrong!
I think your update overwrite your previous changes, which added pointer_type_in_context? Now you're just adding pointer_type_is_opaque (but the tests still use the other function as well).
Hi, thanks for taking the time to look at my patch! Unfortunately, I'm new to Phabricator and Arcanist and the documentation confused me. I'm used to the Git workflow of making a branch and committing and pushing changes. I tried to "update" my patch with arc diff --update, but it seems that this didn't do what I wanted. What should I do to "squash" my two commits into one patch? Thank you for your time and patience!
I've updated the patch by making a new feature branch and squash merging the old one into the new one. Now, both my commits are included in the diff. Was there a better way for me to have done this?
Sounds like the right thing to do in this case. Phabricator doesn't do multi-commit reviews, you need one review per commit (though they can be dependent).
llvm/test/Bindings/OCaml/core.ml | ||
---|---|---|
47 | Hm, does this test really pass? It's not possible to mix opaque and typed pointers in the same context. The second call should be creating an opaque pointer as well. |
do we even need pointer_type_is_opaque? at this point users should assume that all pointers are opaque
llvm/bindings/ocaml/llvm/llvm.mli | ||
---|---|---|
730 |
It turns out that @nikic was correct to be fishy by pointing out that a "test" I added should not have passed! I figured out that I added the test function, but I had not added it to the list of tests at the bottom of the file, so it never ran. When I did run it, the test did indeed fail. I've updated the diff to fix this, as well as to add the missing word "space" in a doc comment that was pointed out.
llvm/test/Bindings/OCaml/core.ml | ||
---|---|---|
47 | It turns out that test should not have passed! It turned out I wasn't actually running the test I added because I neglected to add it to the list of tests at the bottom. When I added it, the test indeed failed. I've updated the patch to fix the test. Your expectation is right; both explicitly created opaque pointer and the pointer created from an i8 type are opaque. |
Also, as I update this patch using the "make a new branch and squash merge" method, my reviewers' inline comments still appear, but are shown on the "new" version of the patch where I fix the issue, which creates a confusing context. I want to know if I'm doing the Phabricator workflow correctly, or if there is something else I should be doing.
I just updated the diff to remove the PointerType::isOpaque binding I added. The only reason I added that was to write a test case checking if a pointer was opaque, which I had *misunderstood* and written incorrectly, as @nikic pointed out. As @aeubanks pointed out, users should assume that all pointers are opaque now (as LLVM is migrating to opaque pointers), so the isOpaque binding perhaps should not be added.
I just want to merge a patch into the main branch to add OCaml bindings for constructing opaque pointers without passing a pointee type. If one were to decide that isOpaque also needs bindings, I think that can be a separate patch after this one is finished.
Just rearranged the test order at the bottom to match the order of test definition...
Thank you for the approval! I do not believe I have permissions to merge, as I just joined Phabricator to make this patch. According to this: https://www.llvm.org/docs/Phabricator.html#committing-a-change
Once a patch has been reviewed and approved on Phabricator it can then be committed to trunk. If you do not have commit access, someone has to commit the change for you (with attribution). It is sufficient to add a comment to the approved review indicating you cannot commit the patch yourself.
So I assume someone with the right permissions is going to merge this?
Also, will this appear in a minor or patch release of LLVM 15, which adds opaque pointers, or will this only appear in the next major release?
Merged.
Also, will this appear in a minor or patch release of LLVM 15, which adds opaque pointers, or will this only appear in the next major release?
I don't think anybody has plans to backport this, so the next major release.