Use __maybe_unused compiler attribute to fix flakey code-coverage in test_edgesec
#432
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the `__maybe_unused` attribute, which prevents warnings when a variable is unused (e.g. if we use the variable only if a given `#ifdef X` is set). In the edgesec project, we normally use a cast to `(void)` to do the same thing, e.g. `(void)my_var;`, but code taken from hostap uses `__maybe_unused`. This is equivalent to the C23 attribute [`[[maybe_unused]]`][1]. [1]: https://en.cppreference.com/w/c/language/attributes/maybe_unused
Add a CMake INTERFACE/header-only library for `src/radius/common.h`
Add `./src/utils/attributes.h` header file for compiler attributes. For example, `__attribute__((unused))` is supported on GCC and Clang, but on other compilers, this is not supported. Most of these are already in `src/radius/common.h`, so we just need to move them into a new file to use them in the rest of the edgesec code.
Move `__maybe_unused` from the `src/radius/common.h` file into `src/utils/attributes.h`, so that we can use it in the rest of the edgesec project.
Use the `__maybe_unused` compiler attribute to mark unused function parameters in `tests/test_edgesec.c` as unused, instead of `(void)`. For some reason, code coverage sometimes thinks that `(void)` lines are not covered by tests. Using `__maybe_unused` instead should fix this issue.
Codecov Report
@@ Coverage Diff @@
## main #432 +/- ##
==========================================
+ Coverage 53.09% 53.14% +0.04%
==========================================
Files 144 144
Lines 19876 19897 +21
==========================================
+ Hits 10553 10574 +21
Misses 9323 9323
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
mereacre
approved these changes
Feb 21, 2023
There was a problem hiding this comment.
Great.
We should also add https://github.com/nqminds/edgesec/blob/main/src/utils/os.h#L36 i.e.,
__attribute__((packed))to the attributes.h file.
Move the `#define STRUCT_PACKED __attribute__((packed))` from `src/utils/os.h` into `src/utils/attributes.h`. Co-authored-by: Alexandru Mereacre <mereacre@gmail.com>
Good idea, I've done it in f770f44 |
Draft
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some reason, these lines are sometimes not run by tests, and because of that, we get a code-coverage warning:
edgesec/tests/test_edgesec.c
Lines 44 to 45 in 9b53dea
I would have thought that these no-op statements are optimized out, even when compiling in debug mode, but I guess sometimes they are kept in?
To fix this, I've taken the
__maybe_unusedmacro from PR #380 and stuck it in a new file calledsrc/utils/attributes.h, which we can use for compiler attributes.(C23 has built-in support for attributes like
[[maybe_unused]], but since we're not using that, we're stuck with doing complicated compiler-specific checks in massive header files)