-
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
Logger: define virtual constructor/method #5208
Conversation
When I create my own implementation of Logger, and try to link it into my executable, I get the error ``` undefined reference to `typeinfo for rocksdb::Logger' ``` I am using rocksdb built in release mode, which seemingly is built with no RTTI. I got it by calling `sudo apt-get install librocksdb-dev`. This change allows folks using such release builds to successfully link their applications. It is similar to #3008
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Done |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Definition of ~Logger is now moved into env.h
Thank you for your contribution! Can you fix the test failures? You can replace |
I think the Windows build fails because it doesn't like redefinition. It there a way to remove from the redifnition of |
The problem is that I will have to fully move the definition of Logv from env.cc into env.h. Logv is a long'ish function that I am hesitant to move into env.h (which just holds definitions or trampoline functions). I am still thinking of ideas. Let me know if you have any. void Logger::Logv(const InfoLogLevel log_level, const char* format, va_list ap) {
static const char* kInfoLogLevelNames[5] = { "DEBUG", "INFO", "WARN",
"ERROR", "FATAL" };
if (log_level < log_level_) {
return;
}
if (log_level == InfoLogLevel::INFO_LEVEL) {
// Doesn't print log level if it is INFO level.
// This is to avoid unexpected performance regression after we add
// the feature of log level. All the logs before we add the feature
// are INFO level. We don't want to add extra costs to those existing
// logging.
Logv(format, ap);
} else if (log_level == InfoLogLevel::HEADER_LEVEL) {
LogHeader(format, ap);
} else {
char new_format[500];
snprintf(new_format, sizeof(new_format) - 1, "[%s] %s",
kInfoLogLevelNames[log_level], format);
Logv(new_format, ap);
}
} |
Can you create a non-virtual function Logger::LogvDefault() or something like that, and let Logger::Logv() call it in env.h? |
The problem is that, if you do that, then all Logger subclasses (like NullLogger, StderrLogger, etc) MUST re-implement Logv(...) because their inherited one is of no use. |
With your approach and users don't implement |
@ugorji do you still plan to work on it, or should we close? |
Let me know if this is still relevant. |
When I create my own implementation of Logger,
and try to link it into my executable, I get the error
I am using rocksdb built in release mode, which seemingly
is built with no RTTI. I got it by calling
sudo apt-get install librocksdb-dev
.This change allows folks using such release builds
to successfully link their applications.
It is similar to #3008