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

MOVEONLY-ISH: allocators: split allocators and pagelocker #5810

Closed
wants to merge 1 commit into from

Conversation

theuni
Copy link
Member

@theuni theuni commented Feb 20, 2015

Part of a larger effort to clean up dependencies, this is a straightforward change. Headers were fixed up as necessary, and I added the bitcoin-config.h include in pagelocker.cpp in case it was picked up from elsewhere before.

Pagelocker is only used for sensitive info, so don't require it for things that just want zero-after-free. In particular, this un-tethers streams from boost and global app-state.

@paveljanik
Copy link
Contributor

utACK, I like this style of cleanup.

@theuni
Copy link
Member Author

theuni commented Feb 20, 2015

nits addressed.

@jtimon
Copy link
Contributor

jtimon commented Feb 20, 2015

ut ACK

@laanwj
Copy link
Member

laanwj commented Feb 23, 2015

Agree on the idea, although I'm wondering whether src/allocators should be a first-class directory or this stuff should be in the more general src/support.

@theuni
Copy link
Member Author

theuni commented Feb 23, 2015

@laanwj To clarify, what do you intend src/support to house?

Either way, I'm happy to move it if you'd prefer it there.

@jtimon
Copy link
Contributor

jtimon commented Feb 23, 2015

Maybe we can move some utils there or maybe we will find out what else to move there later. Just thinking out loud...

@laanwj
Copy link
Member

laanwj commented Feb 25, 2015

@theuni Let me try: src/support is for OS support that means lower-level utility functions that depend on calls to the OS, with potential side effects.

  • Currently cleanse is in there, which makes sense.
  • Allocators also fall squarely into that category (also isn't cleanse mostly/only used by the allocators?).
  • Low-level synchronization primitives would also fit here.
  • Pure utility functions (e.g. utilstrencodings) would not belong there. They are OS-independent and should even work in a completely static computational environment like moxiebox.

This is analogous to leveldb's port_* and env_*. There is a slight overlap with compat, although compat deals with (pure) functions that lack from a certain C/C++ library but are part of some standard.

Maybe we should write a plan how we like the tree organized so that these things are more clear. Maybe there are good arguments why allocators makes sense as a category/library of itself, I just think that the default should be to try to fit something into a broad category and not create too many subdirs of src/.

@theuni
Copy link
Member Author

theuni commented Feb 25, 2015

@laanwj Ok, that sounds good. To me, 'support' implied that the functionality within would be provided by 3rd party libs. Your description makes sense and I follow now, thanks.

If there were more allocators, or more planned, I'd argue that they make sense by themselves. But 2 really isn't enough to justify that.

I'll move and rename to fit.

Pagelocker is only needed for secure (usually wallet) operations, so don't make
the zero-after-free allocator depend on it.
@theuni
Copy link
Member Author

theuni commented Feb 26, 2015

Moved to support.

@jtimon
Copy link
Contributor

jtimon commented Feb 26, 2015

re-utACK

@sipa
Copy link
Member

sipa commented Mar 1, 2015

utACK

@jgarzik
Copy link
Contributor

jgarzik commented Mar 11, 2015

ut ACK

@sipa
Copy link
Member

sipa commented Mar 12, 2015

Needs rebase.

laanwj added a commit that referenced this pull request Mar 20, 2015
d7d187e allocators: split allocators and pagelocker (Cory Fields)
@laanwj
Copy link
Member

laanwj commented Mar 20, 2015

Merged via d7d187e (only conflict was a #include in the tests)

@laanwj laanwj closed this Mar 20, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants