-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Add macros to include file name and line number during Logging #1990
Conversation
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
macros exist in https://github.com/facebook/rocksdb/pull/1990/files#diff-4e3ab62cad64d4929b71457f9c8dad08 almost all the changes are update the code to use the new macros |
@IslamAbdelRahman updated the pull request - view changes - changes since last import |
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
I perfer to put filename:line always first. Currently the log seems not aligned and hard to spot it. |
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.
LGTM (assuming that the tests pass)
#define TOSTRING(x) STRINGIFY(x) | ||
#define PREPEND_FILE_LINE(FMT) ("[" __FILE__ ":" TOSTRING(__LINE__) "] " FMT) | ||
|
||
// Don't inclide file/line info in HEADER level |
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?
@@ -16,6 +16,42 @@ | |||
#include <string> | |||
#include "port/port.h" | |||
|
|||
// Helper macros that include information about file name and line number | |||
#define STRINGIFY(x) #x |
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.
Can we add ROCKS_ prefix to these macros just in case customers also using them?
#define ROCKS_LOG_HEADER(LGR, FMT, ...) \ | ||
rocksdb::Log(InfoLogLevel::HEADER_LEVEL, LGR, FMT, ##__VA_ARGS__) | ||
|
||
#define ROCKS_LOG_DEBUG(LGR, FMT, ...) \ |
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.
This provide an optimization opportunity: later we can redefine some of these macros to be null in production. For example if we redefine ROCKS_LOG_DEBUG to null when DEBUG_LEVEL is 0, then we save the cost of function call.
@lightmark unfortunately it won't be very easy to do that (it's possible but it means that we will need to to it in runtime, instead of in compile time like in this PR) |
current logging
new logging