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

constexpr DEBUG_ASSERT #4131

Merged
merged 3 commits into from
Jul 24, 2021
Merged

constexpr DEBUG_ASSERT #4131

merged 3 commits into from
Jul 24, 2021

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Jul 21, 2021

Replace the unused conditional assignment by plain if() to make our asserts usable in contsexpr

In case the assertion failed the compiler is already complaining that the assertion is no longer const, because QCritical is not const.

@coveralls
Copy link

coveralls commented Jul 21, 2021

Pull Request Test Coverage Report for Build 1058111453

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.0e**-05%**) to 28.634%

Totals Coverage Status
Change from base Build 1057568566: 1.0e-05%
Covered Lines: 20099
Relevant Lines: 70193

💛 - Coveralls

@daschuer
Copy link
Member Author

Oh, it turns out we can remove the lambdas completely.
Done.

@daschuer daschuer requested a review from Holzhaus July 21, 2021 07:27
@@ -52,11 +52,20 @@ inline void mixxx_release_assert(const char* assertion, const char* file, int li
//
// In release builds, doSomething() is never called!
#ifdef MIXXX_DEBUG_ASSERTIONS_ENABLED
#define DEBUG_ASSERT(cond) ((!(cond)) ? mixxx_debug_assert(#cond, __FILE__, __LINE__, ASSERT_FUNCTION) : mixxx_noop())
#define DEBUG_ASSERT(cond) \
if (Q_UNLIKELY(!(cond))) { \
Copy link
Contributor

Choose a reason for hiding this comment

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

How about replacing (cond) with static_cast<bool>(cond) as suggested by @Holzhaus?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@daschuer
Copy link
Member Author

Unfortunately the error message is a bit weird in case of a compile time assert:

home/sperry/workspace/2.3/src/mixxxapplication.cpp: In constructor ‘MixxxApplication::MixxxApplication(int&, char**)’:
/home/sperry/workspace/2.3/src/mixxxapplication.cpp:79:28:   in ‘constexpr’ expansion of ‘{anonymous}::makei(0)’
/home/sperry/workspace/2.3/src/util/assert.h:57:27: error: call to non-‘constexpr’ function ‘void mixxx_debug_assert(const char*, const char*, int, const char*)’
   57 |         mixxx_debug_assert(#cond, __FILE__, __LINE__, ASSERT_FUNCTION); \
      |         ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sperry/workspace/2.3/src/mixxxapplication.cpp:60:5: note: in expansion of macro ‘DEBUG_ASSERT’
   60 |     DEBUG_ASSERT(i);
      |     ^~~~~~~~~~~~

that is also the case with the std assert(), not our fault

@uklotzde
Copy link
Contributor

@Holzhaus Merge?

@Holzhaus
Copy link
Member

Didn't test this yet, but feel free to merge if you did. Otherwise I can have a look in the next days.

@uklotzde
Copy link
Contributor

LGTM

@uklotzde uklotzde merged commit 60a5845 into mixxxdj:main Jul 24, 2021
@daschuer daschuer deleted the constexpr_assert branch August 1, 2021 11:19
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.

4 participants