Skip to content

Conversation

@aseelegbaria
Copy link
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

virtual void start( rs2::subdevice_model & model )
{
#if BUILD_EASYLOGGINGPP
Copy link
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
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

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
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?

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
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

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
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.


rs2_error* e = nullptr;
rs2_log_to_callback( RS2_LOG_SEVERITY_DEBUG, c_callback, nullptr, &e );
#if BUILD_EASYLOGGINGPP
Copy link
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

@dorodnic
Copy link
Contributor

dorodnic commented May 3, 2020

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.

@aseelegbaria aseelegbaria requested a review from maloel August 26, 2020 12:03
@maloel maloel merged commit 82e6f12 into IntelRealSense: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