Support for disabling asserts at compile time

[jrepp]

First support patch for the diagnostics changes in previous PR.

Some notes for implementors:

• apriltag_config.h is only needed for internal compilation units
• apriltag_config.h is included in apriltag_demo.c because for internal testing (similar to other includes in _demo.c)
• asserts currently print to stderr but I have another commit which makes this configurable at compile time
• I also have some other downstream changes for this that address a few issues such as changing exit/panic behavior etc at compile time.

This change only includes the change from assert to AT_ASSERT for compile-time configuration of asserts (including disabling them).

Testing

$ mkdir build && cd build
$ cmake -G Ninja ..
$ ninja
$ ./apriltag_demo -f foo
Unrecognized tag family name. Use e.g. "tag36h11".
apriltag_demo: /home/jrepp/dev/apriltag/example/apriltag_demo.c:105: main: Assertion `0' failed.
Aborted (core dumped)

$ ccmake ..
(toggle assert off)
$ ninja
$ ./apriltag_demo -f foo
$ ./apriltag_demo -f foo
Unrecognized tag family name. Use e.g. "tag36h11".
$ echo $?
255

[christian-rauch]

These two commits should be squashed because the second commit fixes something from the first commit.

This PR also touches a lot of unrelated parts. There are a lot of changes in the includes and reordering in the CMakeLists.txt. It appears to me that these changes are also unrelated to the main purpose of this PR, which is changing the assertion behaviour. Those assertion-unrelated parts should be reviewed independently in a dedicated PR.

The assertions should be disabled in Release mode by default. So, if this is only about disabling assertions, then building in Release mode would be the right way. Or am I missing something here?

How is AT_ASSERT implemented in this PR? It is not obvious to me.

I think this will also break the make script (Makefile). We either have to deprecate and remove the Makefile (https://github.com/AprilRobotics/apriltag/issues/210), or we have to support it.

[jrepp]

These two commits should be squashed because the second commit fixes something from the first commit.

I get what you're saying about always having a single commit - but from the point of the PR or people reviewing the PR that's not what you usually want.

Are you concerned with the commit history of the main branch? If that's the case you can't you just use the squash merge option when accepting the PR?

For example, if you request a change to the PR and I make said change, I shouldn't rewrite that commit, or the review history is borked and anyone trying to follow along or reproduce my results can no longer rebase.

That said I'm happy to conform to whatever makes you most comfortable.

The assertions should be disabled in Release mode by default. So, if this is only about disabling assertions, then building in Release mode would be the right way. Or am I missing something here?

I need a bit more flexibility than that as I'm compiling to different embedded platforms. On embedded platforms, we need the ability to create different flavor builds with and without asserts - release builds with asserts are really useful for testing different code generation options (optimization levels etc).

Additionally, we need to control both assert and panic behavior and I can't defer that to the standard library. For example, in a follow-up commit, I will be able to close this issue: https://github.com/AprilRobotics/apriltag/issues/182 as we will be able to replace exit() and print with some compile-time configuration macros in diagnostic.h (controlled through apriltag_config.h)

[christian-rauch]

@jrepp Are you still interested in this? Looking at the proposed changes and the discussion, you should clearly split this PR into the assertion-related parts and those that just change the code style, fix typos, etc.

Regarding the assertions, I am still not convinced that what you propose is necessary. If you really need assertions in Release mode, why don't you define the corresponding defines? I think NDEBUG is responsible for enabling/disabling the assert macro. Otherwise, you could just redefine the assert macro for release builds when a certain CMake option is provided.

[jrepp]

I'll

@jrepp Are you still interested in this? Looking at the proposed changes and the discussion, you should clearly split this PR into the assertion-related parts and those that just change the code style, fix typos, etc.

agreed

Regarding the assertions, I am still not convinced that what you propose is necessary. If you really need assertions in Release mode, why don't you define the corresponding defines? I think NDEBUG is responsible for enabling/disabling the assert macro. Otherwise, you could just redefine the assert macro for release builds when a certain CMake option is provided.

That's definitely a simple and existing solution. I don't see anything wrong with that.

The reason I often break out defines under flags like this is to allow flexibility. Globally then you can either hide this flag behind NDEBUG or you can select it in a target. I like to have a release debug build as well as a debug profiled build for example.

I'm going to close this PR :)