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

Moving ShMem persisting to take an owned value #2649

Merged
merged 7 commits into from
Nov 3, 2024

Conversation

riesentoaster
Copy link
Contributor

Small change, makes it much more convenient to use. You'll typically call persist_for_child_processes during creation, and if the method takes a borrow, it requires two statements. Now, it can be done in one.

@domenukk
Copy link
Member

domenukk commented Nov 2, 2024

I see why this is nicer to call, but it feels a bit odd, right? I mean, this is not a builder (yet) but a normal method, kinda..
But I don't have strong opinions, either, up to you.

@riesentoaster
Copy link
Contributor Author

riesentoaster commented Nov 2, 2024

I see where you are coming from. I feel like as long as it is only a single thing it's alright. Alternatives to the custom method would be:

  1. Have a second constructor that does this during creation (so besides the current new_shmem add a method new_persistent_shmem)
  2. Have a boolean flag in new
  3. Always call the functionality it on unix systems (but performance and added dependencies on external functionality)

Overall this is probably a pretty niche thing, so maybe it doesn't really matter, and having it in its own function where most people don't have to bother actually has its upsides.

@domenukk
Copy link
Member

domenukk commented Nov 2, 2024

I'd say let's just go with that option 2, and just add a persistent_for_child_processes() (or just persistent?) constructor alongside new().

@domenukk
Copy link
Member

domenukk commented Nov 2, 2024

(they can both call down to an internal constructor that takes a boolean, but exposing that boolean wouldn't be very rust-y I think)

@riesentoaster
Copy link
Contributor Author

I've now created a custom constructor that calls the "normal" constructor and then sets the flags. What do you think?

@domenukk
Copy link
Member

domenukk commented Nov 3, 2024

Sounds good

@domenukk
Copy link
Member

domenukk commented Nov 3, 2024

thanks!

@domenukk domenukk merged commit d4fbe17 into AFLplusplus:main Nov 3, 2024
99 checks passed
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.

2 participants