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

[rttr] Disable -Werror in order to support android #37406

Merged
merged 6 commits into from
Mar 18, 2024

Conversation

FrankXie05
Copy link
Contributor

Fix #37123

Note:
In C++, if two values of different types are operated on or compared, the compiler attempts an implicit type conversion to convert one value to the type of the other. This implicit type conversion may result in loss of precision, overflow, or incorrect results。
Now change to the result of explicit conversion for android.

Upstream PR: rttrorg/rttr#371

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@FrankXie05 FrankXie05 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Mar 13, 2024
@FrankXie05 FrankXie05 marked this pull request as draft March 13, 2024 07:25
@dg0yt
Copy link
Contributor

dg0yt commented Mar 13, 2024

@FrankXie05
Copy link
Contributor Author

Regarding the precision of the values, they are well below the precision range of float(-2^128 ~ +2^128).

(aka 'int') to 'float' changes value from -2147483647 to -2147483648
(aka 'long') to 'float' changes value from 9223372036854775807 to 9223372036854775808

@FrankXie05 FrankXie05 marked this pull request as ready for review March 14, 2024 06:13
@dg0yt
Copy link
Contributor

dg0yt commented Mar 14, 2024

Regarding the precision of the values, they are well below the precision range of float(-2^128 ~ +2^128).

(aka 'int') to 'float' changes value from -2147483647 to -2147483648
(aka 'long') to 'float' changes value from 9223372036854775807 to 9223372036854775808

Precision is not min/max value. Precision is number of signifcant bits. It is relevant for floating point type which use part of the bits for the exponent.
https://en.wikipedia.org/wiki/Floating-point_arithmetic
So there can be integer numbers which are within the min/max range of a given floating point type but cannot be represented exactly.

In addition, the ranges of int, float, and log are not defined exactly. Only the minimum range can be assumed for each type.

Now the patched function is only to indicate if a value is too small or too large to fit into the floating point type. Maybe the patch is acceptable because precision is not an issue here. But correct transition between fp and integer can be tricky, and so it should be reviewed carefully.

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Mar 15, 2024
@Cheney-W
Copy link
Contributor

Add reviewed tag to request further review from Billy.

@BillyONeal BillyONeal added depends:upstream-changes Waiting on a change to the upstream project and removed info:reviewed Pull Request changes follow basic guidelines labels Mar 16, 2024
@BillyONeal
Copy link
Member

I'm not convinced that this patch is correct in the face of all types F and T. In particular, it's not necessarily true that numeric_limits<T>::max is representable in F. The correctness of this depends on the intended interface of the function in question and how it is used.

I do not think it is appropriate to apply a change like this without upstream having made the change first. It is not clearly only build system related or similarly related to our core competencies, so I don't think even a waiting period is appropriate.

If the actual build failure is due to some sort of warning that fires on Android, turning off the warning would be OK.

@BillyONeal BillyONeal changed the title [rttr] support android [rttr] Disable -Werror in order to support android Mar 18, 2024
@BillyONeal BillyONeal merged commit 3850888 into microsoft:master Mar 18, 2024
16 checks passed
@BillyONeal
Copy link
Member

Awesome, thanks!

@FrankXie05 FrankXie05 deleted the dev/Frank/37123 branch March 19, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support depends:upstream-changes Waiting on a change to the upstream project info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rttr] Build error on x64-android
4 participants