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 sugar to set fields with PointerWrapper #77

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

lcw
Copy link
Contributor

@lcw lcw commented Apr 3, 2023

This enables the setting of the user_pointer field of a p4est. This is useful to access julia data in p4est's callback functions. For example:

conn = p4est_connectivity_new_brick(2, 2, 0, 0)
p4est = p4est_new_ext(MPI.COMM_WORLD, conn, 0, 2, 0, 0, C_NULL, C_NULL)
pw = PointerWrapper(p4est)
pw.user_pointer = pointer_from_objref(Ref((3,4)))

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@177d26e). Click here to learn what that means.
The diff coverage is 80.00%.

❗ Current head 1f5b561 differs from pull request most recent head b00fec5. Consider uploading reports for the commit b00fec5 to get more accurate results

@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage        ?   15.63%           
=======================================
  Files           ?        3           
  Lines           ?     1516           
  Branches        ?        0           
=======================================
  Hits            ?      237           
  Misses          ?     1279           
  Partials        ?        0           
Flag Coverage Δ
unittests 15.63% <80.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pointerwrappers.jl 86.36% <80.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Having setproperty! as analog of getproperty is a nice idea.

@JoshuaLampert Could you please please have a look at this and merge it if it looks good to you?

@sloede
Copy link
Member

sloede commented Apr 4, 2023

Looks good from my side, thanks for the contribution!

I wonder if it would be necessary to also add one or two examples to the docs for correctly using this feature. AFAIU, one has to GC-preserve any variables before getting their pointer, at least if the variable is not used afterwards anymore, such that the memory address remains valid. This is of course not an issue of PW but if Julia itself, however, since we expose some very low level stuff with a nice convenience layer, less experienced users might fall into hard-to-debug traps.

I am thinking of maybe one example for a persistent variable to show general usage, and maybe one for a local variable where GC preserve is necessary for defined behavior.

What do the others think?

@JoshuaLampert
Copy link
Member

When this is documented this LGTM. Thanks!

This enables the setting of the `user_pointer` field of a `p4est`.  This
is useful to access julia data in p4est's callback functions. For
example:
```
conn = p4est_connectivity_new_brick(2, 2, 0, 0)
p4est = p4est_new_ext(MPI.COMM_WORLD, conn, 0, 2, 0, 0, C_NULL, C_NULL)
pw = PointerWrapper(p4est)
pw.user_pointer = pointer_from_objref(Ref((3,4)))
```
@lcw
Copy link
Contributor Author

lcw commented Apr 11, 2023

Okay, I added a minimal example.

docs/src/introduction.md Outdated Show resolved Hide resolved
@JoshuaLampert
Copy link
Member

I just realized: Does this also work with these weird structs as in #72 or should we also take a similar approach as in #74?

@ranocha ranocha enabled auto-merge (squash) April 11, 2023 13:55
@ranocha ranocha disabled auto-merge April 11, 2023 13:55
@ranocha ranocha enabled auto-merge (squash) April 11, 2023 13:55
@ranocha
Copy link
Member

ranocha commented Apr 11, 2023

I just realized: Does this also work with these weird structs as in #72 or should we also take a similar approach as in #74?

Good point. Now that #74 is merged, I think we can proceed like this.

  1. Merge this PR. It already gives a nice new feature, so we can go ahead.
  2. You can check it afterwards and make a PR if required.

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.

4 participants