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

replace mod_inv_25 by explicit value as a fix for VS warning C4307 #3450

Merged
merged 1 commit into from May 18, 2023

Conversation

florimond-collette
Copy link
Contributor

@florimond-collette florimond-collette commented May 17, 2023

revert non-working workaround for issue #3163

This fix for warning C4307: '*': integral constant overflow leads to warning C4309: 'static_cast': truncation of constant value, which is not better (both are level 3 VS warnings). Revert to the original code that is clearer and also present further in this source code.

Alternatively, to avoid any VS warning, one could follow the third suggestion of #3163 (comment) and write the explicit value of mod_inv_25 which is 0xc28f5c29 and replace the lines with:
const uint32_t mod_inv_25 = 0xc28f5c29 // = mod_inv_5 * mod_inv_5;

@vitaut
Copy link
Contributor

vitaut commented May 17, 2023

Could you apply the alternative suggestion to suppress the warning? i.e.

const uint32_t mod_inv_25 = 0xc28f5c29 // = mod_inv_5 * mod_inv_5;

@florimond-collette
Copy link
Contributor Author

The value of mod_inv_25 is now explicit in 32 and 64 bit:
const uint32_t mod_inv_25 = 0xc28f5c29; // = mod_inv_5 * mod_inv_5
const uint64_t mod_inv_25 = 0x8f5c28f5c28f5c29; // = mod_inv_5 * mod_inv_5

@vitaut
Copy link
Contributor

vitaut commented May 18, 2023

Please rebase.

@florimond-collette florimond-collette changed the title revert non-working workaround for issue #3163 replace mod_inv_25 by explicit value as a fix for VS warning C4307 May 18, 2023
@vitaut vitaut merged commit d60b907 into fmtlib:master May 18, 2023
76 checks passed
@vitaut
Copy link
Contributor

vitaut commented May 18, 2023

Thanks

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.

None yet

2 participants