-
Notifications
You must be signed in to change notification settings - Fork 38.7k
[LevelDB] Plug leveldb logs to bitcoin logs #9999
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
Conversation
|
Nice! utACK 9d21779 |
src/dbwrapper.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd prefer not to use vsnprintf here. We've purged this from the code when introducing tinyformat. Wonder if we can do something based on our strprintf. Though bridging between C varargs and C++ is likely going to be difficult, and this solution as-taken-from LevelDB should be fine. It's just fairly ugly code and should not be taken as a hint to contributors that using system v?s?n?printf is okay in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add a comment in that regard, mentioning where the code comes from and why and explaining the above.
9d21779 to
75fac86
Compare
|
Added some comments about the ugly vsprintf. Bridging varargs to use |
src/dbwrapper.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CBitcoinLevelDBLogger maybe? The name is too general otherwise, we should reserve this class name for ourselves.
5aa4de6 to
00bd866
Compare
|
Renamed. Can Travis be restarted ? I do not think the error is related to this PR. |
src/dbwrapper.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this class in the .h file? It seems to be only used internally in dbwrapper.cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, h for declaration and cpp for implementation, at least that is what I always thought. You mean just putting the class directly in the cpp ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.h is for the externally accessible parts of a cpp file. This class doesn't need to be exposed to anything, so it can just go inside the cpp file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, .h is for interface declaration. Although with C++ it's not always clear what should be part of the interface and what not (especially when tests are involved), this one could just as well be private to the .cpp.
src/init.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please retain alphabetical ordering (leveldb should be between http and libevent)
00bd866 to
cfce581
Compare
|
fixed nits |
cfce581 [LevelDB] Plug leveldb logs to bitcoin logs (NicolasDorier) Tree-SHA512: e40a2c2644c269bb2da7be04aec39ff64ad350d508391750a757955ed3f9d96998775d01e04b282a75b36d776c3960a345cc7b6f1466e6ae167d27518bf4baee
cfce581 [LevelDB] Plug leveldb logs to bitcoin logs (NicolasDorier) Tree-SHA512: e40a2c2644c269bb2da7be04aec39ff64ad350d508391750a757955ed3f9d96998775d01e04b282a75b36d776c3960a345cc7b6f1466e6ae167d27518bf4baee
Implementation
copy pasteinspired bybitcoin/src/leveldb/util/posix_logger.h
Line 28 in 57b3459
ping @laanwj