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

Please don't force enable assertions. #1632

Closed
patlefort opened this issue Apr 22, 2023 · 7 comments · Fixed by #1633
Closed

Please don't force enable assertions. #1632

patlefort opened this issue Apr 22, 2023 · 7 comments · Fixed by #1633
Labels

Comments

@patlefort
Copy link

Typically release builds are built with NDEBUG as assertions with assert() are meant for debugging. It should be left to the user compiling the code to decide if they want to live dangerously. If some of these checks are that important (or worse, expected), it shouldn't use assert(). For me it's a pain to have to make another exception when compiling code from the AUR (Arch User Repository).

Environment details (Put x in the checkbox along with the information)

Irrelevant as it affect any environment.

Exact steps to reproduce the issue

Compile with -DNDEBUG.

@patlefort patlefort added the bug label Apr 22, 2023
@N-R-K
Copy link
Collaborator

N-R-K commented Apr 22, 2023

nnn itself doesn't use any asserts. It's only used in the hashtable generator. Originally the generator didn't even use the environment flags, but it does since 2fc9d51.

Honestly didn't think there was any need for the generator to use environmental flags since it won't ever be used by the user. But it's whatever, I'll remove the asserts and replace it with something handmade sometime soon.

@patlefort
Copy link
Author

Are you saying it is only used during the build process? In any case, simply removing the lines at

#error "The hash-table generator relies on assert() to verify correctness."
would solve the issue or undefining NDEBUG before including any files.

@N-R-K
Copy link
Collaborator

N-R-K commented Apr 22, 2023

Are you saying it is only used during the build process?

Yes. And now that I think about it, using CC for the generator isn't right, it won't work for cross compile situation since the generator needs to be built and run on the host machine not the target.

On gentoo the convention seems to be use BUILD_{CC,CFLAGS,LDFLAGS} for situations like this. I'll probably take a look into what other distros do and fix accordingly.

simply removing the lines at

That would just disable the checks, which is not a proper solution.

@patlefort
Copy link
Author

It could simply be a separate project/makefile that's compiled and installed on the host machine. On Arch, we have make dependencies, it would be very simple to manage.

@jarun
Copy link
Owner

jarun commented Apr 23, 2023

@N-R-K is it OK to remove those?

@N-R-K
Copy link
Collaborator

N-R-K commented Apr 23, 2023

is it OK to remove those?

@jarun Nah the checks shouldn't be removed. I have a commit ready fixing the issue. Will open a PR sometime today hopefully (been pretty sick lately).

@jarun
Copy link
Owner

jarun commented Apr 23, 2023

Take your time! Get well soon!

N-R-K added a commit to N-R-K/nnn that referenced this issue Apr 23, 2023
since 2fc9d51, the hash-table generator inherits environmental
CFLAGS and so we shouldn't disallow setting -DNDEBUG.

fixes: jarun#1632
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants