-
Notifications
You must be signed in to change notification settings - Fork 224
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
logging: Replace NOLOGGING #362
Conversation
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.
While you're at it: To me it seems that the logger passes strings by value where we might want to have const refs. While SSO will deal with setLogLevel
, the messages in log
, logToTerminal
should be const refs.
CMakeLists.txt
Outdated
@@ -18,6 +18,7 @@ option(NETWORKIT_COVERAGE "Build with support for coverage" OFF) | |||
set(NETWORKIT_PYTHON "" CACHE STRING "Directory containing Python.h. Implies MONOLITH=TRUE") | |||
set(NETWORKIT_PYTHON_SOABI "" CACHE STRING "Platform specific file extension. Implies MONOLITH=TRUE") | |||
set(NETWORKIT_WITH_SANITIZERS "" CACHE STRING "Uses sanitizers during the compilation") | |||
set(NETWORKIT_RELEASE_LOGGING "AUTO" CACHE STRING "Do not compile log messages at levels TRACE or DEBUG") |
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 add available values (AUTO|ON|OFF) to description.
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.
Good point.
@@ -37,7 +37,7 @@ class AuxGTest: public testing::Test{}; | |||
|
|||
TEST_F(AuxGTest, produceRandomIntegers) { | |||
Aux::Random::setSeed(1, false); | |||
#if (LOG_LEVEL == LOG_LEVEL_TRACE) | |||
#ifndef NETWORKIT_RELEASE_LOGGING |
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.
It seems that a more CPPish way of achieving this would be to define a constexpr in Global.h
and replace the preprocessor commands using CPP ifs. Any decent compiler will remove the unreachable code. Or am I missing something?
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.
AFAICS the main use of the #ifdef
is to prevent unused variable warnings when TRACE
or DEBUG
is compiled out. A if with constant argument will not help in this case as the dead code elimination will only be done in the compiler backend, after the unused variable warning has already been issued.
That being said, a lot of the code using the #if LOGLEVEL == [...]
idiom is quite questionable. I decided to only do syntactical changes to LOG_LEVEL
in this PR but we can definitely take another look at those cases after the PR is merged.
@@ -8,7 +8,7 @@ endif("${PROJECT_SOURCE_DIR}" STREQUAL "${PROJECT_BINARY_DIR}") | |||
# BUILD OPTIONS | |||
option(NETWORKIT_BUILD_CORE "Build NetworKit core library" ON) | |||
option(NETWORKIT_BUILD_TESTS "Build NetworKit C++ tests" OFF) | |||
option(NETWORKIT_LOGGING "Build with logging support" ON) |
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.
Should we first mark NETWORKIT_LOGGING
as DEPREACTED and issue an error message hinting to the new options if it is ever set to OFF?
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.
I am not sure if it's worthwhile to do this. The option is not used in any standard build (e.g., setup.py
) and it is quite easy to adapt to the new option. If you think that it is worthwhile, I suggest to fail the build if the option is specified so that users are forced to adapt their builds.
networkit/cpp/auxiliary/Log.cpp
Outdated
@@ -41,6 +43,8 @@ std::string getLogLevel() { | |||
return "ERROR"; | |||
} else if (current == LogLevel::fatal) { | |||
return "FATAL"; | |||
} else if (current == LogLevel::quiet) { |
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.
It seems that this if-block should be a switch-case
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.
Makes sense.
While NOLOGGING utilized the C++ preprocessor to remove calls to logging functions at compile time, NETWORKIT_QUIET_LOGGING disables logging at runtime, *unless* users explicitely set a log level. We achieve this by introducing a new log level called QUIET. QUIET does not have corresponding logging functions; however, when the current log level is set to QUIET, no messages (of any level) are emitted.
Introduce a preprocessor definition that can be set to disable the compilation of logging calls for levels TRACE and DEBUG. Adjust CMakeLists.txt to automatically set this behavior in Release mode and introduce a configuration option NETWORKIT_RELEASE_LOGGING to control it. Using this option, users are still able to configure release builds with full logging (e.g., to verify that a new feature works correctly in release mode).
LOG_LEVEL is not really used anymore. Replace LOG_LEVEL >= TRACE and LOG_LEVEL >= DEBUG by !NETWORKIT_RELEASE_LOGGING.
NOLOGGING is not used anymore.
This avoids a copy when passing std::string objects from Impl::log() to logToTerminal() / logToFile().
Tidy up the code to use a switch instead of an if-chain.
05cc5b9
to
a5636f1
Compare
I rebased the patch series onto the current |
The |
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 👍
Thank you for the review. |
As mentioned in #361 (comment), the current behavior of the
-NOLOGGING
processor definition (and the corresponding CMake option-DNETWORKIT_LOGGING=OFF
) is problematic.This PR fixes the issue by splitting the responsibilities of
-DNOLOGGING
into two parts:Library users who want to hide the logging functionaltiy of NetworKIT should now set
-DNETWORKIT_QUIET_LOGGING
(instead of-DNETWORKIT_LOGGING=OFF
) during configuration.