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

Avoid unwanted sign extensions from MSVC in is_utf8. #2297

Merged
merged 2 commits into from
May 19, 2021

Conversation

mwinterb
Copy link
Contributor

Microsoft's constexpr evaluator treats the type of micro[0] and micro[1] as
plain char, and so sign extends before comparing them to ints.
The normal compiler, including the optimizer, does not fail in this way,
so this is merely a "future proof" change in case someone uses is_utf8()
in a constant expression.

Microsoft's constexpr evaluator treats the type of micro[0] and micro[1] as
plain char, and so sign extends before comparing them to ints.
The normal compiler, including the optimizer, does not fail in this way,
so this is merely a "future proof" change in case someone uses is_utf8()
in a constant expression.
@mwinterb
Copy link
Contributor Author

The smallest repro pointing out the issue is this: https://gcc.godbolt.org/z/xTxPfzqz9. /J is how Microsoft spells -funsigned-char. All compiles should return 1 from main() to indicate agreement between constant evaluation and "runtime" evaluation, but mostly default options do not do that.
I couldn't find any decent places to forcibly test this as none of the tests specify /utf-8, and it seems ODR-violating to just compile the tests with that and rude to add the option unconditionally when building the static library.
Another fix is just to make is_utf8 non-constexpr, but keeping the comment that it's explicitly non-constexpr to avoid Microsoft's bug.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good, just two nits.

Comment on lines 394 to 395
// avoid buggy sign extensions in MSVC's constant evaluation mode
// https://developercommunity.visualstudio.com/t/C-difference-in-behavior-for-unsigned/1233612
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please make it a proper sentence:

  // Avoid buggy sign extensions in MSVC's constant evaluation mode
  // https://developercommunity.visualstudio.com/t/C-difference-in-behavior-for-unsigned/1233612.

include/fmt/core.h Outdated Show resolved Hide resolved
@vitaut vitaut merged commit 2a2e4c5 into fmtlib:master May 19, 2021
@vitaut
Copy link
Contributor

vitaut commented May 19, 2021

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.

2 participants