Skip to content

Fix a bug in format localized number on s390x #1871

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

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

samding01
Copy link
Contributor

@samding01 samding01 commented Feb 1, 2019

When building Foundation test on s390x, there is a warning:

   CoreFoundation/String.subproj/CFString.c:6030:36: warning: result of comparison of constant -1 with expression of type 'char' is always false [-Wtautological-constant-out-of-range-compare]

The debugging shows (disableLocalizedFormatting == -1) is never true because disableLocalizedFormatting is defined as char, and then the logic of format localized number is not correct on s390x.

Changing disableLocalizedFormatting to short to make the logic works on all platforms

@spevans
Copy link
Contributor

spevans commented Feb 1, 2019

@swift-ci test

@compnerd
Copy link
Member

compnerd commented Feb 1, 2019

@samding01 ... I assume that is due to the use of -funsigned-char. I think that the better thing to do here might be to pass in -fsigned-char to the CoreFoundation build, since it does make that assumption in a bunch of places. Alternatively, use _Bool rather than changing this to a short.

@spevans
Copy link
Contributor

spevans commented Feb 1, 2019

Doesn't it need to hold one of 3 values though, so _Bool wouldn't be sufficient? Or maybe just make it an int?

@samding01 samding01 force-pushed the master_s390x_foundation branch from 9fb8ecb to f9f75d2 Compare February 1, 2019 18:10
@samding01
Copy link
Contributor Author

Thanks @compnerd and @spevans Updated to int

@spevans
Copy link
Contributor

spevans commented Feb 3, 2019

@swift-ci test

@samding01
Copy link
Contributor Author

Can you review and test?

@compnerd
Copy link
Member

@swift-ci please test and merge

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