Skip to content

Conversation

@NicolasDorier
Copy link
Contributor

@NicolasDorier NicolasDorier commented Mar 15, 2017

Implementation copy paste inspired by

virtual void Logv(const char* format, va_list ap) {

ping @laanwj

@laanwj
Copy link
Member

laanwj commented Mar 15, 2017

Nice! utACK 9d21779

Copy link
Member

@laanwj laanwj Mar 15, 2017

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.

Copy link
Member

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.

@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Mar 15, 2017

Added some comments about the ugly vsprintf. Bridging varargs to use strprintf is difficult and error prone. I do not think it is worth it.

src/dbwrapper.h Outdated
Copy link
Member

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.

@NicolasDorier NicolasDorier force-pushed the leveldblog branch 3 times, most recently from 5aa4de6 to 00bd866 Compare March 15, 2017 12:42
@NicolasDorier
Copy link
Contributor Author

Renamed. Can Travis be restarted ? I do not think the error is related to this PR.

src/dbwrapper.h Outdated
Copy link
Member

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.

Copy link
Contributor Author

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 ?

Copy link
Member

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.

Copy link
Member

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
Copy link
Contributor

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)

@NicolasDorier
Copy link
Contributor Author

fixed nits

@laanwj laanwj merged commit cfce581 into bitcoin:master Mar 18, 2017
laanwj added a commit that referenced this pull request Mar 18, 2017
cfce581 [LevelDB] Plug leveldb logs to bitcoin logs (NicolasDorier)

Tree-SHA512: e40a2c2644c269bb2da7be04aec39ff64ad350d508391750a757955ed3f9d96998775d01e04b282a75b36d776c3960a345cc7b6f1466e6ae167d27518bf4baee
@maflcko maflcko mentioned this pull request Mar 19, 2017
12 tasks
@jnewbery jnewbery mentioned this pull request Apr 5, 2017
codablock pushed a commit to codablock/dash that referenced this pull request Oct 24, 2017
cfce581 [LevelDB] Plug leveldb logs to bitcoin logs (NicolasDorier)

Tree-SHA512: e40a2c2644c269bb2da7be04aec39ff64ad350d508391750a757955ed3f9d96998775d01e04b282a75b36d776c3960a345cc7b6f1466e6ae167d27518bf4baee
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants