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: Fix -Werror=nonnull-compare warning in thread affinity assertion #10863

Merged
merged 1 commit into from
Sep 3, 2022

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Sep 3, 2022

Using gcc 12.2.0, passing a nonnull argument as parameter to the
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY macro leads to the following
compiler warning (or error if the build system was configured with
-DWARNINGS_FATAL=ON):

In file included from mixxx/src/preferences/configobject.h:11,
                 from mixxx/src/util/sandbox.h:10,
                 from mixxx/src/library/coverart.h:7,
                 from mixxx/src/library/coverartcache.h:9,
                 from mixxx/src/library/libraryfeature.h:15,
                 from mixxx/src/library/baseexternallibraryfeature.h:7,
                 from mixxx/src/library/baseexternallibraryfeature.cpp:1:
mixxx/src/library/trackcollection.h: In member function ‘PlaylistDAO& TrackCollection::getPlaylistDAO()’:
mixxx/src/util/assert.h:55:39: error: ‘nonnull’ argument ‘this’ compared to NULL [-Werror=nonnull-compare]
   55 | #define DEBUG_ASSERT(cond) ((!(cond)) ? mixxx_debug_assert(#cond, __FILE__, __LINE__, ASSERT_FUNCTION) : mixxx_noop())
      |                            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mixxx/src/util/thread_affinity.h:14:5: note: in expansion of macro ‘DEBUG_ASSERT’
   14 |     DEBUG_ASSERT(pObject);                            \
      |     ^~~~~~~~~~~~
mixxx/src/library/trackcollection.h:56:9: note: in expansion of macro ‘DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY’
   56 |         DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mixxx/src/library/trackcollectionmanager.h: In member function ‘TrackCollection* TrackCollectionManager::internalCollection()’:
mixxx/src/util/assert.h:55:39: error: ‘nonnull’ argument ‘this’ compared to NULL [-Werror=nonnull-compare]
   55 | #define DEBUG_ASSERT(cond) ((!(cond)) ? mixxx_debug_assert(#cond, __FILE__, __LINE__, ASSERT_FUNCTION) : mixxx_noop())
      |                            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mixxx/src/util/thread_affinity.h:14:5: note: in expansion of macro ‘DEBUG_ASSERT’
   14 |     DEBUG_ASSERT(pObject);                            \
      |     ^~~~~~~~~~~~
mixxx/src/library/trackcollectionmanager.h:40:9: note: in expansion of macro ‘DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY’
   40 |         DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [CMakeFiles/mixxx-lib.dir/build.make:2624: CMakeFiles/mixxx-lib.dir/src/library/baseexternallibraryfeature.cpp.o] Error 1

This can easily be solved by not asserting that the pointer is not
nullptr. Even if that macro is used with a regular (= not nonnull
pointer), that assetion is unnecessary because the worst that can happen
is a crash (which also makes the developer aware of a problem). When
debug assertions are disabled, the resulting instructions are the same
anyway.

Using gcc 12.2.0, passing a `nonnull` argument as parameter to the
`DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY` macro leads to the following
compiler warning (or error if the build system was configured with
`-DWARNINGS_FATAL=ON`):

    In file included from mixxx/src/preferences/configobject.h:11,
                     from mixxx/src/util/sandbox.h:10,
                     from mixxx/src/library/coverart.h:7,
                     from mixxx/src/library/coverartcache.h:9,
                     from mixxx/src/library/libraryfeature.h:15,
                     from mixxx/src/library/baseexternallibraryfeature.h:7,
                     from mixxx/src/library/baseexternallibraryfeature.cpp:1:
    mixxx/src/library/trackcollection.h: In member function ‘PlaylistDAO& TrackCollection::getPlaylistDAO()’:
    mixxx/src/util/assert.h:55:39: error: ‘nonnull’ argument ‘this’ compared to NULL [-Werror=nonnull-compare]
       55 | #define DEBUG_ASSERT(cond) ((!(cond)) ? mixxx_debug_assert(#cond, __FILE__, __LINE__, ASSERT_FUNCTION) : mixxx_noop())
          |                            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    mixxx/src/util/thread_affinity.h:14:5: note: in expansion of macro ‘DEBUG_ASSERT’
       14 |     DEBUG_ASSERT(pObject);                            \
          |     ^~~~~~~~~~~~
    mixxx/src/library/trackcollection.h:56:9: note: in expansion of macro ‘DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY’
       56 |         DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    mixxx/src/library/trackcollectionmanager.h: In member function ‘TrackCollection* TrackCollectionManager::internalCollection()’:
    mixxx/src/util/assert.h:55:39: error: ‘nonnull’ argument ‘this’ compared to NULL [-Werror=nonnull-compare]
       55 | #define DEBUG_ASSERT(cond) ((!(cond)) ? mixxx_debug_assert(#cond, __FILE__, __LINE__, ASSERT_FUNCTION) : mixxx_noop())
          |                            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    mixxx/src/util/thread_affinity.h:14:5: note: in expansion of macro ‘DEBUG_ASSERT’
       14 |     DEBUG_ASSERT(pObject);                            \
          |     ^~~~~~~~~~~~
    mixxx/src/library/trackcollectionmanager.h:40:9: note: in expansion of macro ‘DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY’
       40 |         DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    cc1plus: all warnings being treated as errors
    make[2]: *** [CMakeFiles/mixxx-lib.dir/build.make:2624: CMakeFiles/mixxx-lib.dir/src/library/baseexternallibraryfeature.cpp.o] Error 1

This can easily be solved by not asserting that the pointer is not
`nullptr`. Even if that macro is used with a regular (= not `nonnull`
pointer), that assetion is unnecessary because the worst that can happen
is a crash (which also makes the developer aware of a problem). When
debug assertions are disabled, the resulting instructions are the same
anyway.
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@Swiftb0y Swiftb0y merged commit a2b7367 into mixxxdj:2.3 Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants