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

Replace QtDebug-based DEBUG_ASSERT implementation with C++ assert() #4132

Closed
wants to merge 1 commit into from

Conversation

Holzhaus
Copy link
Member

No description provided.

@github-actions github-actions bot added the build label Jul 21, 2021
@daschuer
Copy link
Member

Can you describe which way the assert string goes now? Does it still end in the mixxx.log file?

@Holzhaus
Copy link
Member Author

Nope, it just prints to the terminal and exits. That's the limitation.

https://en.cppreference.com/w/cpp/error/assert

@daschuer
Copy link
Member

Mm, no good news.

What fixes this PR in addition to my branch?
Do we really need that comma feature?

Can you give an example, wat we can do with assert() and what fails with our implementation?

Can we adopt the solution from std to our version.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1052730655

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage remained the same at 29.028%

Files with Coverage Reduction New Missed Lines %
src/library/proxytrackmodel.cpp 1 0%
src/util/logging.cpp 1 67.74%
Totals Coverage Status
Change from base Build 1052728631: 0%
Covered Lines: 20091
Relevant Lines: 69213

💛 - Coveralls

#define VERIFY_OR_DEBUG_ASSERT(cond) \
DEBUG_ASSERT(cond); \
if (false)
#define RELEASE_ASSERT(cond) assert(static_cast<bool>(cond))
Copy link
Contributor

Choose a reason for hiding this comment

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

@daschuer Can we adopt this static_cast for #4131 to prevent mistakes like using an assignment operator instead of equality comparison?

@uklotzde
Copy link
Contributor

How about improving the current solution as proposed in #4131 and then as a second step prepare the migration to the assert macro?

Having the output of a debug assertion appear in the log file helps to analyze bug reports from users who are not familiar with using the command line and a debugger.

@Holzhaus
Copy link
Member Author

How about improving the current solution as proposed in #4131 and then as a second step prepare the migration to the assert macro?

My original idea was to make the whole DEBUG_ASSERT infrastructure a bit easier. But this seems not feasible if we keep the requirement to log the assertion into the logfile (which is useful without a doubt).

If @daschuer fixed the issues in #4131, I'll just close this for now.

@Holzhaus Holzhaus closed this Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants