Skip to content

BUILD_EASYLOGGING = OFF - #6304

Merged
maloel merged 8 commits into
realsenseai:developmentfrom
aseelegbaria:easyloggingflag
Aug 27, 2020
Merged

BUILD_EASYLOGGING = OFF #6304
maloel merged 8 commits into
realsenseai:developmentfrom
aseelegbaria:easyloggingflag

Conversation

@aseelegbaria

Copy link
Copy Markdown
Contributor

compiles with -DBUILD_EASYLOGGINGPP=OFF and unit_tests pass
issue number: #6009

@maloel
maloel changed the base branch from master to development April 27, 2020 10:12
Comment thread common/post-processing-filter.h Outdated

virtual void start( rs2::subdevice_model & model )
{
#if BUILD_EASYLOGGINGPP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be necessary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this to the end and then let's talk about it

Comment thread src/rs.cpp Outdated
void rs2_log_to_callback_cpp( rs2_log_severity min_severity, rs2_log_callback * callback, rs2_error** error ) BEGIN_API_CALL
{
// Wrap the C++ callback interface with a shared_ptr that we set to release() it (rather than delete it)
#if BUILD_EASYLOGGINGPP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have the exception already happening inside log.cpp, then no need to do it here?

Comment thread src/rs.cpp Outdated
void rs2_log_to_callback( rs2_log_severity min_severity, rs2_log_callback_ptr on_log, void * arg, rs2_error** error ) BEGIN_API_CALL
{
// Wrap the C function with a callback interface that will get deleted when done
#if BUILD_EASYLOGGINGPP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same -- can be done in one central place

Comment thread src/rs.cpp Outdated
const char* rs2_get_full_log_message( rs2_log_message const* msg, rs2_error** error ) BEGIN_API_CALL
{
VALIDATE_NOT_NULL( msg );
#if BUILD_EASYLOGGINGPP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define a log_message API that will centralize these #if statements?

I.e., it would look like:

log_message & wrapper = *( log_message * )( msg );
return wrapper.get_full_log_message();

And the log_message object (in log.h) would then know to throw if ELPP is not enabled.

Comment thread unit-tests/log/test-c-debug.cpp Outdated

rs2_error* e = nullptr;
rs2_log_to_callback( RS2_LOG_SEVERITY_DEBUG, c_callback, nullptr, &e );
#if BUILD_EASYLOGGINGPP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we arrange it so all these tests aren't even compiled and run if ELPP is off? We can make a single new test that tests that log_to_callback throws, instead

Comment thread src/log.cpp
@dorodnic

dorodnic commented May 3, 2020

Copy link
Copy Markdown
Contributor

General comment - having a bunch of #ifdefs all over the code is bad practice.
Perhaps instead dynamic or static polymorphism can be used?
Dynamic - define logger class with virtual functions. Select inheriting class based on single #ifdef.
Static - define template class logger. Select template parameter in a single #ifdef.

Comment thread src/log.h Outdated
Comment thread src/log.h Outdated
Comment thread tools/realsense-viewer/CMakeLists.txt Outdated
Comment thread tools/realsense-viewer/realsense-viewer.cpp Outdated
Comment thread tools/depth-quality/rs-depth-quality.cpp
Comment thread src/device_hub.cpp Outdated
Comment thread src/l500/ac-trigger.cpp Outdated
@aseelegbaria
aseelegbaria requested a review from maloel August 26, 2020 12:03
@maloel
maloel merged commit 82e6f12 into realsenseai:development Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants