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.

Sign up for GitHub

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

Jump to bottom

Conditionally enable non-portable assert on LP64. #312

Merged
merged 1 commit into from
Dec 12, 2016
Merged

Conditionally enable non-portable assert on LP64. #312

merged 1 commit into from
Dec 12, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Dec 10, 2016 •
edited
Loading

This only works on LP64. It may fail on Windows, which can be LLP64
(IL32P64).

This change is Reviewable

@iphydf iphydf added this to the v0.1.0 milestone Dec 10, 2016
iphydf added a commit to iphydf/c-toxcore that referenced this pull request Dec 10, 2016
@iphydf
Remove non-portable assert.
59e9d52 
This only works on LP64. It may fail on Windows, which can be LLP64
(IL32P64).

Fixes TokTok#312.
@robinlinden
Copy link
Member

:lgtm_strong:

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

iphydf added a commit to iphydf/c-toxcore that referenced this pull request Dec 10, 2016
@iphydf
Remove non-portable assert.
d3f9c21 
This only works on LP64. It may fail on Windows, which can be LLP64
(IL32P64).

Fixes TokTok#312.
isotoxin
isotoxin reviewed Dec 10, 2016
View reviewed changes
Copy link

@isotoxin isotoxin left a comment

Choose a reason for hiding this comment

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

This code compiled by Visual Studio ok.
No need to remove these asserts

@robinlinden
Copy link
Member

Yeah, I think we should leave these in too. They fired when they should have.

@iphydf iphydf added the P1 High priority label Dec 11, 2016
@GrayHatter
Copy link

I'm with @robinlinden on this as well. They did do their job. I was having issues with them because they were killing my profile when the padding was incorrect. having tested #313 it now works correctly.

If it becomes a problem again, we should drop it, until then. It did help us catch the padding error.

@GrayHatter
Copy link

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

@iphydf iphydf changed the title Remove non-portable assert. Conditionally enable non-portable assert on LP64. Dec 12, 2016
@iphydf
Conditionally enable non-portable assert on LP64.
f208fb5 
This only works on LP64. It may fail on Windows, which can be LLP64
(IL32P64).
@iphydf
Copy link
Member Author

iphydf commented Dec 12, 2016

@robinlinden PTAL.

@iphydf iphydf merged commit f208fb5 into TokTok:master Dec 12, 2016
@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

:lgtm_strong:

Reviewed 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

@iphydf iphydf deleted the nonportable-assert branch December 23, 2016 02:30
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isotoxin isotoxin isotoxin left review comments

Assignees

@robinlinden robinlinden

Labels
P1 High priority
Projects
None yet
Milestone
v0.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@iphydf @robinlinden @GrayHatter @nurupo @isotoxin