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

Introduce internal includes section to avoid potential implicit declaration warnings #219

Merged

Conversation

ycherniavskyi
Copy link
Contributor

During the integration of the libcanard into a project based on ChibiOS I want to change CANARD_ASSERT from the default assert to ChibiOS osalDbgCheck. It is done with the next C defines -DCANARD_ASSERT=osalDbgCheck -DCANARD_CONFIG_HEADER=\"${BINDINGS_DIR}/canard/canard_config.h\", there to prevent implicit declaration warnings I unutilized CANARD_CONFIG_HEADER by providing a canard_config.h file with the following content:

#include "osal.h"

But because of #include "_canard_cavl.h" and # include CANARD_CONFIG_HEADER order in canard.c I still get implicit declaration warnings:

Compiling canard.c
../../ext/OpenCyphal/libcanard/libcanard/_canard_cavl.h: In function 'cavlPrivateRotate':
<command-line>: warning: implicit declaration of function 'osalDbgCheck' [-Wimplicit-function-declaration]
../../ext/OpenCyphal/libcanard/libcanard/_canard_cavl.h:30:25: note: in expansion of macro 'CANARD_ASSERT'
   30 | #    define CAVL_ASSERT CANARD_ASSERT
      |                         ^~~~~~~~~~~~~
../../ext/OpenCyphal/libcanard/libcanard/_canard_cavl.h:98:5: note: in expansion of macro 'CAVL_ASSERT'
   98 |     CAVL_ASSERT((x != NULL) && (x->lr[!r] != NULL) && ((x->bf >= -1) && (x->bf <= +1)));
      |     ^~~~~~~~~~~

To resolve this issue # include CANARD_CONFIG_HEADER must be placed before #include "_canard_cavl.h".

Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Thank you. At first, I thought it violates AUTOSAR C++ M16-0-1, but upon closer inspection, I see that this solution is compliant.

I tried to push the following corrections before merging but your fork is not writable for me; please apply them yourself and request another review:

diff --git a/README.md b/README.md
index 4ebc819..f651750 100644
--- a/README.md
+++ b/README.md
@@ -241,6 +241,11 @@ If you find the examples to be unclear or incorrect, please, open a ticket.
 
 - Refactor the transfer reassembly state machine to enhance its maintainability and robustness.
 
+#### v3.1.2
+
+- Allow redefinition of CANARD_ASSERT via the config header;
+  see [#219](https://github.com/OpenCyphal/libcanard/pull/219).
+
 ### v3.0
 
 - Update branding as [UAVCAN v1 is renamed to Cyphal](https://forum.opencyphal.org/t/uavcan-v1-is-now-cyphal/1622).
diff --git a/libcanard/canard.c b/libcanard/canard.c
index 5cd28aa..863de50 100644
--- a/libcanard/canard.c
+++ b/libcanard/canard.c
@@ -38,6 +38,7 @@
 #endif
 
 // --------------------------------------------- INTERNAL INCLUDES ----------------------------------------------
+// The internal includes are placed here after the config header is included and CANARD_ASSERT is defined.
 
 #include "_canard_cavl.h"
 

@pavel-kirienko pavel-kirienko merged commit 1c4da0d into OpenCyphal:master Dec 26, 2023
6 checks passed
@ycherniavskyi ycherniavskyi deleted the introduce-internal-includes-section branch February 21, 2024 20:47
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.

2 participants