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

Add custom assert() (PROF_ASSERT()) #1929

Closed
wants to merge 1 commit into from

Conversation

H3rnand3zzz
Copy link
Contributor

@H3rnand3zzz H3rnand3zzz commented Nov 12, 2023

See commit message.

Valgrind not needed in this case.

How to test the functionality

  • Cause assert failure or add custom assert that will certainly fail.

Resolve #1895

Add custom macro to assert statement and log error
prior to closing application,
it allows us to handle `assert()`'s with a grace and track issues much better.

Dependencies are updated in each file, "trace.h" is placed correctly
near imports of similar nature.

In 3 cases return statement were added alongside assert(FALSE)
since compiler don't recognize 'abort()' in case of statement failure.

In 1 case 2 imports of `<assert.h>` were replaced by 1 include of `trace.h`

Resolves profanity-im#1895
@H3rnand3zzz
Copy link
Contributor Author

@jubalh @sjaeckel @pasis, if someone kindly can find a time to review it, it would be amazing.

@jubalh
Copy link
Member

jubalh commented Dec 15, 2023

Personally I fail to see the reason for it.
For this reason, plus pasis being the author of pppoat2, I thought it's best if pasis and sjaeckel review it.

@H3rnand3zzz
Copy link
Contributor Author

Personally I fail to see the reason for it. For this reason, plus pasis being the author of pppoat2, I thought it's best if pasis and sjaeckel review it.

The reason is on plain site: to make debugging easier. If someone has assert problem, he can provide profanity.log containing a line that shows where the problem has happened. Otherwise we need coredump, which not all the users can provide.

@jubalh
Copy link
Member

jubalh commented Dec 15, 2023

That's exactly what I mean. For me it is default that under Linux we check things using gdb, get backtrace, receive a coredump from users.
For me this and https://profanity-im.github.io/issues.html seems to be enough and seems to be the standard way in the Linux world.
So for me it is not visible why we need this and what downsides it could bring. Which is why I will let someone else review it :)

@jubalh
Copy link
Member

jubalh commented Mar 21, 2024

Not a big fan of this. And since noone else reviewed yet I'll close.

@jubalh jubalh closed this Mar 21, 2024
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.

Change assert behavior
2 participants