Skip to content
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

Add newline to logs #307

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Conversation

jean-roland
Copy link
Contributor

This fixes #238

In addition, it switches to a compiler based macro, to ensure no obsolete log will blow up 2 years after the renaming of a variable because we didn't build with the correct ZENOH_DEBUG value.

Also added a compiler symbol to identify a debug/release build.

_Z_LOG_PREFIX(ERROR); \
printf(x, ##__VA_ARGS__);
#define _Z_ERROR_CONTINUE(x, ...) printf(x, ##__VA_ARGS__);
#define _Z_DEBUG(...) \

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
#define _Z_INFO(x, ...) (void)(0)
#define _Z_ERROR(x, ...) (void)(0)
#endif
#define _Z_ERROR(...) \

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
#if ZENOH_DEBUG == 0 && !defined(Z_BUILD_DEBUG)

#define _Z_DEBUG(...) (void)(0)
#define _Z_INFO(...) (void)(0)

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
@@ -17,55 +17,57 @@

#include <stdio.h>

// Logging values
#define _Z_LOG_LVL_ERROR 1
#define _Z_LOG_LVL_INFO 2

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
// Ignore print only if log deactivated and build is release
#if ZENOH_DEBUG == 0 && !defined(Z_BUILD_DEBUG)

#define _Z_DEBUG(...) (void)(0)

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
_Z_LOG_PREFIX(ERROR); \
printf(x, ##__VA_ARGS__);
#define _Z_ERROR_CONTINUE(x, ...) printf(x, ##__VA_ARGS__);
#define _Z_INFO(...) \

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file

#define _Z_DEBUG(...) (void)(0)
#define _Z_INFO(...) (void)(0)
#define _Z_ERROR(...) (void)(0)

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
printf(x, ##__VA_ARGS__);
#define _Z_ERROR_CONTINUE(x, ...) printf(x, ##__VA_ARGS__);
// Ignore print only if log deactivated and build is release
#if ZENOH_DEBUG == 0 && !defined(Z_BUILD_DEBUG)

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2009 with no text in the supplied rule-texts-file Warning

misra violation 2009 with no text in the supplied rule-texts-file
@@ -17,55 +17,57 @@

#include <stdio.h>

// Logging values
#define _Z_LOG_LVL_ERROR 1

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
// Logging values
#define _Z_LOG_LVL_ERROR 1
#define _Z_LOG_LVL_INFO 2
#define _Z_LOG_LVL_DEBUG 3

Check warning

Code scanning / Cppcheck (reported by Codacy)

misra violation 2101 with no text in the supplied rule-texts-file Warning

misra violation 2101 with no text in the supplied rule-texts-file
Copy link
Member

@cguimaraes cguimaraes left a comment

Choose a reason for hiding this comment

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

Some clarification required.

printf(__VA_ARGS__); \
printf("\n"); \
} \
} while (false)
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this do..while if the loop only executes once?

Copy link
Contributor Author

@jean-roland jean-roland Dec 28, 2023

Choose a reason for hiding this comment

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

It turns the block into a single statement and forces a ; after the macro. If you want more info on this: https://stackoverflow.com/questions/4674480/do-whilefalse-pattern

Copy link
Member

Choose a reason for hiding this comment

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

If MISRA-C is still followed for the codebase, this shall not be required.
But I get the point...

include/zenoh-pico/utils/logging.h Show resolved Hide resolved
include/zenoh-pico/utils/logging.h Show resolved Hide resolved
#define _Z_ERROR_CONTINUE(x, ...) printf(x, ##__VA_ARGS__);
#define _Z_DEBUG(...) \
do { \
if (ZENOH_DEBUG >= _Z_LOG_LVL_DEBUG) { \
Copy link
Member

Choose a reason for hiding this comment

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

Why to do this check at runtime, instead of at compile time as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it transitions from a pre-compiler thing to a compile time check, as the compiler realises this values won't change for the whole execution of the program, a bit like a C++ constexpr. But just to be extra sure I added the (void)(0) in release mode and no debug, in case some exotic compiler emits run time checks.

The problem with pre compiler stuff is that it doesn't check if the code is valid while the compiler do. So let's say in my log statement I print a variable, that is then changed (type or name), it will go unnoticed until the compiler sees it, so if I only compile without Debug it won't see it. Now it will always see this kind of issues.

Copy link
Member

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 is already in place, but having compilation tests for both debug and release seems a simpler option.

include/zenoh-pico/utils/logging.h Show resolved Hide resolved
include/zenoh-pico/utils/logging.h Show resolved Hide resolved
@Mallets Mallets merged commit be7f92c into eclipse-zenoh:master Jan 9, 2024
47 checks passed
@jean-roland jean-roland deleted the ft_log_newline branch January 9, 2024 14:51
@jean-roland jean-roland mentioned this pull request Apr 5, 2024
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.

Make the logging macros handle newlining
3 participants