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 extra NULL argument at the end of fail* APIs #298

Merged
merged 1 commit into from
Aug 3, 2020
Merged

Conversation

brarcher
Copy link
Contributor

@brarcher brarcher commented Aug 2, 2020

The fail, fail_unless, and fail_if APIs were expected to contain
a message explaining the failure. This was never enforced, and
it was possible to write unit tests without providing messages.

In #249 a change was introduced to add printf argument checking to the
Check assert APIs, including the fail APIs. There were a few fixes for this
in https://github.com/libcheck/check/releases/tag/0.15.1. Those changes
proved problematic for the uses of the fail* APIs without arguments,
as those uses were now flagged as missing the necessary arguments.

A fix proposed by heftig in https://github.com/libcheck/check/issues/293
is to add a new NULL to the end of every fail* call in the macro
itself. For users of these APIs who do pass a message there will
be a new warning about too many arguments. As the fail APIs are
deprecated, this new warning is a reasonable trade-off, and can
be avoided by switching fail* calls to ck_assert* calls.

If a fail* APIs is passing a message, the emitted warning might be:

check_check_sub.c:65:23: warning: too many arguments for format [-Wformat-extra-args]
   fail_unless(1 == 2, "This test should fail");
                       ^
../src/check.h:472:77: note: in definition of macro ‘fail_unless’
     _ck_assert_failed(__FILE__, __LINE__, "Assertion '"#expr"' failed" , ## __VA_ARGS__, NULL)
                                                                             ^~~~~~~~~~~

The fail, fail_unless, and fail_if APIS were expected to contain
a message explaining the failure. This was never enforced, and
it was possible to write unit tests without providing messages.

In github.com//pull/249 a change was
introduced to add printf argument checking to the Check assert
APIS, including the fail APIs. There were a few fixes for this
in github.com/libcheck/check/releases/tag/0.15.1. Those changes
proved problematic for the uses of the fail* APIs without arguments,
as those uses were now flagged as missing the necessary arguments.

A fix proposed by heftig in github.com//issues/293
is to add a new NULL to the end of every fail* call in the macro
itself. For users of these APIs who do pass a message there will
be a new warning about too many arguments. As the fail APIs are
deprecated, this new warning is a reasonable trade-off, and can
be avoided by switching fail* calls to ck_assert* calls.
@brarcher brarcher merged commit 4ed1ae1 into master Aug 3, 2020
@mikkoi
Copy link
Contributor

mikkoi commented Aug 3, 2020

The commit is question is "Add macro CK_ATTRIBUTE_FORMAT" 5e2b32f

I wanted to use GCC's facilities (since they are available).
But this fix is for all users, not just GCC users. It might react differently in different compilers.

But if that part of the API is deprecated anyway, it might work even if it might cause some extra headache.

Otherwise, just get rid of my commit "Add macro CK_ATTRIBUTE_FORMAT". It was wishful thinking on my part. :-)

brarcher added a commit that referenced this pull request Aug 8, 2020
@brarcher brarcher deleted the fail-extra-null branch January 1, 2021 01:26
bjosv added a commit to Nordix/r3 that referenced this pull request Sep 22, 2023
Fixes [-Wformat-extra-args] warnings when built using GCC,
See details in:
libcheck/check#298
bjosv added a commit to Nordix/r3 that referenced this pull request Oct 26, 2023
Fixes [-Wformat-extra-args] warnings when built using GCC,
See details in:
libcheck/check#298
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