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

Use <stdlib.h> for alloca on FreeBSD. #723

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 22, 2018

https://www.freebsd.org/cgi/man.cgi?alloca

Some additional fixed had to be done as the Windows build started failing once I fixed the freebsd build.


This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Jan 22, 2018
@iphydf iphydf force-pushed the alloca-freebsd branch 12 times, most recently from d1aebb0 to 9e2d1c2 Compare January 22, 2018 19:57
@robinlinden
Copy link
Member

:lgtm_strong:


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Jan 22, 2018

Reviewed 7 of 9 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


other/travis/toxcore-script, line 13 at r1 (raw file):

}

# Try some combinations of cmake options.

why is this removed? not needed anymore?


toxcore/Messenger.c, line 2261 at r1 (raw file):

            uint8_t filenumber = data[0];

#if UINT8_MAX > MAX_CONCURRENT_FILE_PIPES

What happens in the case of UINT8_MAX < MAX_CONCURRENT_FILE_PIPES? looks a bit like a hack to pass tests for me


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Jan 22, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


toxcore/Messenger.c, line 2261 at r1 (raw file):

Previously, sudden6 wrote…

What happens in the case of UINT8_MAX < MAX_CONCURRENT_FILE_PIPES? looks a bit like a hack to pass tests for me

This is not to pass tests. The condition below is always false if the cpp condition is false. filenumber is a uint8_t, so can never be larger than 255. This causes compiler warnings and I'd like to avoid those. This has nothing to do with tests.

If UINT8_MAX < MAX..., then that means we can't even index the highest filenumber. I don't think that's possible without changing the type of filenumber in a lot of places.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Jan 22, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


other/travis/toxcore-script, line 13 at r1 (raw file):

Previously, sudden6 wrote…

why is this removed? not needed anymore?

It has never caught any bugs, and seriously slows down windows and freebsd builds.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Jan 22, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions.


toxcore/Messenger.c, line 2261 at r1 (raw file):

Previously, iphydf wrote…

This is not to pass tests. The condition below is always false if the cpp condition is false. filenumber is a uint8_t, so can never be larger than 255. This causes compiler warnings and I'd like to avoid those. This has nothing to do with tests.

If UINT8_MAX < MAX..., then that means we can't even index the highest filenumber. I don't think that's possible without changing the type of filenumber in a lot of places.

Ok, can we enforce UINT8_MAX >= MAX_CONCURRENT_FILE_PIPES by a #error + comment? Would make sense against accidental changes IMO.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Jan 22, 2018

Reviewed 1 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

https://www.freebsd.org/cgi/man.cgi?alloca

If stdlib.h does not define alloca, and we're using GCC (or Clang), we
define the macro ourselves in terms of a GCC builtin.
@iphydf
Copy link
Member Author

iphydf commented Jan 22, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion.


toxcore/Messenger.c, line 2261 at r1 (raw file):

Previously, sudden6 wrote…

Ok, can we enforce UINT8_MAX >= MAX_CONCURRENT_FILE_PIPES by a #error + comment? Would make sense against accidental changes IMO.

Done.


Comments from Reviewable

@sudden6
Copy link

sudden6 commented Jan 22, 2018

:lgtm_strong:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit 8f1bbcf into TokTok:master Jan 22, 2018
@iphydf iphydf deleted the alloca-freebsd branch January 22, 2018 21:52
This pull request was closed.
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