include: When defining NDEBUG, assert will implement alignment standards

[Gary-Hobson]

Summary

As defined by the C standard, if NDEBUG is defined, assert should do nothing but be simply defined as:

#define assert(ignore) ((void)0)

Reference link:
https://pubs.opengroup.org/onlinepubs/009695399/basedefs/assert.h.html

Impact

Testing

[acassis]

@Gary-Hobson after your modification microADB is failing here:

    if (!uv_is_active((uv_handle_t*)&client->read_pipe)) {
        /* Restart read events */
        int ret = uv_read_start((uv_stream_t*)&client->read_pipe,
            usb_uv_allocate_frame,
            usb_uv_on_data_available);
        /* TODO check return code */
        assert(ret == 0);
        UNUSED(ret);
    }

Seems like you need to replace "#define assert(f) (0)" with something like "#define assert(f) (f && 0)".

This is strange because UNUSED() should avoid the issue, but is not working!

[fjpanag]

I thought that the internal assertions, like ASSERT() and VERIFY() (not the standard assert()), were controlled by CONFIG_DEBUG_ASSERTIONS and not NDEBUG.

Also before ASSERT() was always active, and only DEBUGASSERT() was enabled/disabled depending on the build type.

Is this changed now?

[xiaoxiang781216]

I thought that the internal assertions, like ASSERT() and VERIFY() (not the standard assert()), were controlled by CONFIG_DEBUG_ASSERTIONS and not NDEBUG.

No, DEBUGASSERT is controlled by CONFIG_DEBUG_ASSERTIONS. ASSERT, VERIFY and assert are controlled by NDEBUG.

Also before ASSERT() was always active, and only DEBUGASSERT() was enabled/disabled depending on the build type.

Is this changed now?

this patch doesn't change the assertion behavior, only the warning of unused variable may generate in the new patch if you use assert, but it's required by spec.

[xiaoxiang781216]

@Gary-Hobson after your modification microADB is failing here:

    if (!uv_is_active((uv_handle_t*)&client->read_pipe)) {
        /* Restart read events */
        int ret = uv_read_start((uv_stream_t*)&client->read_pipe,
            usb_uv_allocate_frame,
            usb_uv_on_data_available);
        /* TODO check return code */
        assert(ret == 0);
        UNUSED(ret);
    }

Seems like you need to replace "#define assert(f) (0)" with something like "#define assert(f) (f && 0)".

can't not, since the spec require that assert shouldn't reference f when NDEBUG is defined

This is strange because UNUSED() should avoid the issue, but is not working!

Yes. @Gary-Hobson please find the reason why UNUSED doesn't work as expect.

[fjpanag]

this patch doesn't change the assertion behavior, only the warning of unused variable may generate in the new patch if you use assert, but it's required by spec.

This is not correct.

Please see how the ASSERT() definition was moved from here to here, i.e. the macro was always active before and now it is conditionally active based on NDEBUG.

If ASSERT() follows NDEBUG, then what is its difference with assert()? They are the same thing. Then why not just remove ASSERT() completely?

But as it has been discussed before in the mailing list, it is considered beneficial for the kernel to have its own assertion macros, and have the ability to control them independently from the apps.

[fjpanag]

This is not correct.

Please see how the ASSERT() definition was moved from here to here, i.e. the macro was always active before and now it is conditionally active based on NDEBUG.

Sorry, I checked again the code and it seems that I am wrong. Indeed the behavior is not changing now.

As I understand this change has already happened here.

However, the rest of the points I think that still hold. What is the value of having both ASSERT() and assert() now?

[xiaoxiang781216]

This is not correct.
Please see how the ASSERT() definition was moved from here to here, i.e. the macro was always active before and now it is conditionally active based on NDEBUG.

Sorry, I checked again the code and it seems that I am wrong. Indeed the behavior is not changing now.

As I understand this change has already happened here.

However, the rest of the points I think that still hold. What is the value of having both ASSERT() and assert() now?

ASSERT() is an internal api, so we can always reference the argument and avoid use UNUSED macro in many places.
assert() is defined by standard, so we can't reference the argument if NDEBUG equals true and have to add UNUSED macro.

[Gary-Hobson]

@Gary-Hobson after your modification microADB is failing here:

    if (!uv_is_active((uv_handle_t*)&client->read_pipe)) {
        /* Restart read events */
        int ret = uv_read_start((uv_stream_t*)&client->read_pipe,
            usb_uv_allocate_frame,
            usb_uv_on_data_available);
        /* TODO check return code */
        assert(ret == 0);
        UNUSED(ret);
    }

Seems like you need to replace "#define assert(f) (0)" with something like "#define assert(f) (f && 0)".

This is strange because UNUSED() should avoid the issue, but is not working!

There is no UNUSED in the source code of microADB. After I added it to the code, the compilation warning disappeared.

This PR ignores the compilation warning in microADB.
apache/nuttx-apps#2381

This warning has been fixed in the latest version of microADB
https://github.com/spiriou/microADB/pull/41/files

[xiaoxiang781216]

@Gary-Hobson let's update microADB in apps to the last version.