Implement logging #180
Implement logging #180
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
ddc9afa to
679318e
Compare
ohm314
left a comment
There was a problem hiding this comment.
In my opinion there is still a lot to be done before this PR can be considered for merging. Just to give a few high-level remarks:
- The PR should come along with a clear explanation on the technical choices taken and the changes done. The commit & PR messages are non-existent here.
- The code has to compile and pass tests before merging can be considered
- The code has to adhere to our standard and must be commented
- The PR has to address the issue fully. While a possible logger implementation has been provided (I'm still wondering why this particular logger was chosen), the logger is almost not used anywhere in the code.
Feel free to update the PR to resolve above issues.
| @@ -0,0 +1,6663 @@ | |||
| // | |||
There was a problem hiding this comment.
We don't favor simply including external libraries. Much better would be to add git submodules for such libraries
| #include <mpi.h> | ||
| #endif | ||
|
|
||
| #include "CORENEURON/config.hpp" |
There was a problem hiding this comment.
Not sure what this is referring to?
| static const string reset; | ||
| }; | ||
|
|
||
| const string OUT::black = "\033[30m"; |
There was a problem hiding this comment.
Why would those assignments not be done inside the struct?
| */ | ||
| #define ML_CVLOG_IF(condition, verbose_level, logger_id, x) CVLOG_IF(condition, verbose_level, logger_id) << x | ||
| #else | ||
| #define ML_LOG(level, x) |
There was a problem hiding this comment.
I see here and further below that in your implementation (maybe that's the easylogging way to do it) you heavily rely on macros. I think that's a bad idea. We are trying to modernize this code, moving away from macros and defines as possible. C++ offers other, more powerful and flexible features to build and use APIs.
| namespace CORENEURON | ||
| { | ||
| /** | ||
| * Logging facilities for CORENEURONT++. |
There was a problem hiding this comment.
I'm a bit puzzled about this naming?
|
|
||
| namespace coreneuron | ||
| { | ||
| namespace mpi |
There was a problem hiding this comment.
As in many other places in this PR it's not entirely clear where your choice comes from and what this particular change is there for. There is a lot of documentation / commenting missing
coreneuron/nrniv/main1.cpp
Outdated
| printf("Error: Stop time (%lf) < Start time (%lf), restoring from checkpoint? \n", | ||
| tstop, t); | ||
| abort(); | ||
| ML_LOG(ERROR, "Error: Stop time" << tstop << "Start time" << "restoring from checkpoint? \n"; |
There was a problem hiding this comment.
This code cannot have been compiled. The line above could not compile, I believe.
| abort(); | ||
| } | ||
| ML_LOG(FATAL, "fatal error"); | ||
| } |
| #endif | ||
| { | ||
| abort(); | ||
| ML_LOG(FATAL, "fatal error"); |
There was a problem hiding this comment.
The program will still need to be aborted.
|
No change requests were implemented. No activity for half a year, I'll close this now. |
Fix #95.