Page MenuHomePhabricator

[llvm-ocaml] Add binding for constructing opaque pointers
ClosedPublic

Authored by alan on Sep 29 2022, 3:23 PM.

Diff Detail

Event Timeline

alan created this revision.Sep 29 2022, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 3:23 PM
alan requested review of this revision.Sep 29 2022, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 3:23 PM
alan updated this revision to Diff 464143.Sep 29 2022, 10:49 PM

[llvm-ocaml] Add binding for llvm::PointerType::isOpaque

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).

alan added a comment.Sep 30 2022, 6:32 AM

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!

alan updated this revision to Diff 464253.Sep 30 2022, 6:47 AM

[llvm-ocaml] Add binding for constructing opaque pointers

alan added a comment.Sep 30 2022, 6:48 AM

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?

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

jberdine added inline comments.
llvm/bindings/ocaml/llvm/llvm.mli
730
alan added a comment.Sep 30 2022, 5:12 PM
This comment was removed by alan.
alan updated this revision to Diff 464449.Sep 30 2022, 5:47 PM

[llvm-ocaml] Add binding for constructing opaque pointers

alan marked an inline comment as done.Sep 30 2022, 5:56 PM

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.

alan updated this revision to Diff 464451.Sep 30 2022, 5:59 PM
alan marked an inline comment as done.

[llvm-ocaml] Add binding for constructing opaque pointers

alan added a comment.Sep 30 2022, 6:15 PM

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.

alan updated this revision to Diff 464467.Sep 30 2022, 9:27 PM

[llvm-ocaml] Add binding for constructing opaque pointers

alan added a comment.Sep 30 2022, 9:32 PM

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.

alan updated this revision to Diff 464505.Oct 1 2022, 8:57 AM

[llvm-ocaml] Add binding for constructing opaque pointers

alan added a comment.Oct 1 2022, 8:58 AM

Just rearranged the test order at the bottom to match the order of test definition...

aeubanks accepted this revision.Oct 2 2022, 1:24 PM
This revision is now accepted and ready to land.Oct 2 2022, 1:24 PM
alan marked an inline comment as done.EditedOct 2 2022, 6:11 PM

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?

This revision was automatically updated to reflect the committed changes.

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?

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.