Skip to content

Conversation

@mordante
Copy link
Contributor

@StephanTLavavej here some changes I made in libc++ for https://reviews.llvm.org/D70631 "Microsoft's floating-point to_chars powered by Ryu and Ryu Printf". If you don't like _Output feel free to suggest an alternative and I'll adjust libc++ accordingly.

64-bit types are long for libc++ on the amd64 platform. Explicitly cast
the type to silence the warnings.
On libc++ __output is a reserved name. I'm open to other suggestions.
Note that __out is also a reserved name for libc++.
@mordante mordante requested a review from a team as a code owner February 21, 2021 18:33
@ghost
Copy link

ghost commented Feb 21, 2021

CLA assistant check
All CLA requirements met.

@mordante
Copy link
Contributor Author

I also notices line 1103 and 1105 look oddly similar in their suggestion "Consider retuning PrefixesToTest and FractionBits.". I suspect that's not intended. Both also have a small typo in "returning".

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Feb 22, 2021
@StephanTLavavej StephanTLavavej changed the title To chars libc++ compatibility to_chars libc++ compatibility Feb 22, 2021
@StephanTLavavej
Copy link
Member

I also notices line 1103 and 1105 look oddly similar in their suggestion "Consider retuning PrefixesToTest and FractionBits.". I suspect that's not intended. Both also have a small typo in "returning".

if (ms < 3'000) {
puts("That was fast. Consider retuning PrefixesToTest and FractionBits.");
} else if (ms > 30'000) {
puts("That was slow. Consider retuning PrefixesToTest and FractionBits.");
}

It was intentional, although I can see now how it could be confusing. By "retuning" I meant "re-tuning", and the similar messages were intended to say "if this test ran really fast, you should probably be testing more random values" and "if this test ran really slow, you should probably test fewer random values". I can clarify this.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good to me! I've pushed two small commits, one to restore indentation (clang-format is disabled for the Ryu-derived code so it can stay somewhat consistent with upstream, otherwise it would have detected the extra space) and another to clarify the fast/slow comments (also in another test that had used the same wording).

Our system involves 2 reviews before simultaneously merging to GitHub and the MSVC-internal repo, so we should be able to get a second reviewer and batch this up for merging early next week.

@mordante
Copy link
Contributor Author

Looks good to me! I've pushed two small commits, one to restore indentation (clang-format is disabled for the Ryu-derived code so it can stay somewhat consistent with upstream, otherwise it would have detected the extra space) and another to clarify the fast/slow comments (also in another test that had used the same wording).

The new wording looks clearer, thanks.

Our system involves 2 reviews before simultaneously merging to GitHub and the MSVC-internal repo, so we should be able to get a second reviewer and batch this up for merging early next week.

Great, once it has landed I'll sync the code in libc++.

@StephanTLavavej StephanTLavavej self-assigned this Feb 25, 2021
@StephanTLavavej StephanTLavavej merged commit 6dd9e24 into microsoft:main Feb 26, 2021
@StephanTLavavej
Copy link
Member

Thanks for keeping our codebases aligned, and congratulations on your first microsoft/STL commit! 😸 🎉 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants