openssl: abstract out OPENSSL_cleanse #5689
Conversation
|
Concept ACK. I'm not against one function per object if that makes sense but let's not put all of these at the top level. For organizational purposes could put it in a subdirectory below src, e.g. support or util. |
|
Sorry for the lack of response. I've been putting this off because this is part of a larger reorganization, and I figured I'd wait until it took shape before moving it somewhere since I'm not sure how it will end up looking. Right now, it'd make sense to move this along with the string/time/etc utils to some utils/ dir, but I'd also like to keep the consensus/app-specific stuff segregated in some way. |
|
@laanwj After looking working on a refactor including this for a while, I still don't really have a good place for it. How about leaving it at the top for now and moving it to support/util when there's more stuff that can go there? For ex, we may consider moving random.o and other remaining openssl users into there, so that we have a more straightforward path to removal. Otherwise, I'm happy to move it to whereever you prefer. |
|
Well we are in the process of moving source files from the top level, so I'm not that happy to create more there. As for "let's move it later". If we know we are going to move it I prefer to create it in the right place (this avoids extra rebases as well as diff noise). I don't mind if this is the only file in support/ for now. |
|
Ok, will do. |
|
moved and rebased. |
src/support/cleanse.cpp
Outdated
There was a problem hiding this comment.
4 spaces indentation, pretty please?
There was a problem hiding this comment.
Grr.. still haven't managed to completely break this habit. Will fix.
|
utACK |
src/support/cleanse.cpp
Outdated
There was a problem hiding this comment.
Nit: Can you add a new-line here please?
|
utACK after fixing the comments. |
|
Nits addressed. |
This makes it easier for us to replace it if desired, since it's now only in one spot. Also, it avoids the openssl include from allocators.h, which essentially forced openssl to be included from every compilation unit.
|
Thanks, utACK! |
|
utACK |
1 similar comment
|
utACK |
1630219 openssl: abstract out OPENSSL_cleanse (Cory Fields)
4 participants
This makes it easier for us to replace it if desired, since it's now only in one spot. Also, it avoids the openssl include from allocators.h, which essentially forced openssl to be included from every compilation unit.
Also a few minor header cleanups.
I'm not a big fan of putting a single function in this object, but we use it at such low levels (allocators.h) that i don't want to stick it somewhere that would pull in other headers. Suggestions welcome.