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

Fix a99ac62: fmt's include of cassert breaks our assert logic #9370

Merged
merged 1 commit into from Jun 13, 2021

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Jun 13, 2021

Motivation / Problem

Including <cassert> breaks our asserts and that could lead to e.g. the pool checks to fail as in some cases the side effects of asserts are ran, and in others they are not.

Description

Remove <cassert> from being included in "format-inl.h" and add a comment there so we are unlikely to re add it when updating fmt.

Limitations

None as far as I know.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
PeterN
PeterN approved these changes Jun 13, 2021
@rubidium42 rubidium42 merged commit c811d42 into OpenTTD:master Jun 13, 2021
14 checks passed
Loading
@rubidium42 rubidium42 deleted the fix-asserts branch Jun 13, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 13, 2021

To leave a bit of background:

We define 3 ways of compiling OpenTTD (in relation to asserts):

  • Debug builds: asserts enabled, NDEBUG not defined
  • Pre-release builds (RCs, beta, ..): asserts enabled, NDEBUG defined
  • Release builds: asserts disabled and NDEBUG defined

Now there are all kind of things at play here, but the important thing to know: with Release and RelWithDebInfo as build-type, CMake automatically adds NDEBUG.
Also, cassert will noop assert() if NDEBUG is set.

To allow this "in-between" state for pre-releases, we have WITH_ASSERT. If this is enabled, no matter if NDEBUG is set or not, assert will be defined. We do this in stdafx.h.

Now the final pieces of this puzzle: #include <cassert> includes assert.h which does it at-most to ignore any header-validation and say: fuck you guys, if you include me, I will take over assert().

Normally, the flow is:

  • Every .cpp file includes stdafx.h
  • stdafx.h includes cassert first
  • stdafx.h fixes assert in case of NDEBUG and WITH_ASSERT

Now with fmt it breaks, because:

  • A .cpp file includes stdafx.h and debug.h
  • stdafx.h includes cassert first
  • stdafx.h fixes assert in case of NDEBUG and WITH_ASSERT
  • debug.h file includes fmt-library
  • fmt-library includes cassert again
  • cassert takes over our assert()

This means the game thinks it runs with asserts, but assert() doesn't do anything for files using debug.h but does something for files that do not. This not only gives compiler warnings, but also triggers an assert() at some point, as assert-only code is executed in one file but not in another.

In other words: NEVER include cassert after stdafx.h or we will run into trouble. That also goes for 3rdparty libraries.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants