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

Formatting error for round numbers in add-on manager #3871

Closed
sevu opened this issue Jan 14, 2019 · 8 comments
Closed

Formatting error for round numbers in add-on manager #3871

sevu opened this issue Jan 14, 2019 · 8 comments
Labels
Add-ons Issues with the add-ons client and/or server. Bug Issues involving unexpected behavior. Good first issue Issues deemed adequate for contributors without prior experience to work on. Ready for testing Issues for which a potential fix is available but untested. UI User interface issues, including both back-end and front-end issues.

Comments

@sevu
Copy link
Member

sevu commented Jan 14, 2019

Maybe unrelated to #3870, could proof-check if #1396 is affected or not.
upload
It's 1000 KiB
Not really reproducible I guess ;)

@sevu sevu added Bug Issues involving unexpected behavior. UI User interface issues, including both back-end and front-end issues. Add-ons Issues with the add-ons client and/or server. Good first issue Issues deemed adequate for contributors without prior experience to work on. labels Jan 14, 2019
@jyrkive
Copy link
Member

jyrkive commented Jan 14, 2019

The size is formatted with function si_string_impl_stream_write(). That function uses the C++ standard library default formatting mode on all compilers except Microsoft Visual Studio.

static void si_string_impl_stream_write(std::stringstream &ss, double input) {

I believe the fix would involve using the fixed formatting mode with all compilers.

@sevu
Copy link
Member Author

sevu commented Jan 14, 2019

I used gcc.
Could it be that it's rounded up to 1000 for 995-999, and the workaround only works from 1000?

@jyrkive
Copy link
Member

jyrkive commented Jan 14, 2019

Yes, it's possible that the actual value was slightly less than 1000 kiB. Thus, the precision was only set to three digits, and libstdc++ (GCC's standard library implementation) chose the scientific formatting mode.

We should just use the fixed mode everywhere. Relying on the default mode not using scientific formatting is fragile.

Vultraz added a commit that referenced this issue Jan 22, 2019
@Vultraz Vultraz added the Ready for testing Issues for which a potential fix is available but untested. label Jan 22, 2019
Vultraz added a commit that referenced this issue Jan 22, 2019
nemaara pushed a commit to nemaara/wesnoth that referenced this issue Jan 30, 2019
@Wedge009
Copy link
Member

A pity it's not easy to reproduce. I could check this with MSVC on Windows and gcc on Linux.

@jyrkive
Copy link
Member

jyrkive commented Feb 25, 2019

To test this, you could just hardcode the value to whatever you want.

completed = connection_->current();

@Wedge009
Copy link
Member

Thanks for the pointer. The nice thing is that I don't need to download anything extra - the add-ons list download is sufficient.

I saw the commit that Vultraz did and given that sevu is using gcc I focused on that, but both MSVC and gcc (with a value of 1024000) show 1000.0 KiB instead of 1e+03 KiB now. I'd say this can be closed, assuming sevu is okay with it.

@AI0867
Copy link
Member

AI0867 commented Jun 8, 2019

First off, 1 MiB = 1024 KiB, so the case in the report can be reproduced with values from 1000 KiB to 1023 KiB. (just add std::cerr << utils::si_string(1024000, true, _("unit_byte^B")) << "\n"; to the top of your main).

I tried to keep the number size fairly small and constant, hence the 3-digit precision, bumped to 4 digits for just the above case. This means at all times, at most 4 characters are used for the number.

Unfortunately, MSVC ignores the standard saying that in default mode, the precision method specifies the number of significant digits, and takes it to mean the number of decimals instead. So for MSVC we do the next-best thing and just use fixed mode with 1 decimal, even though this can use up to 6 characters (e.g. 1000.0), and can result in fairly nonsensical precisions (1.0 bytes).

Equivalent printf statements: http://cpp.sh/23ign (according to standard sections 30.7.5.2.2 and 25.4.2.2.2, also: https://en.cppreference.com/w/cpp/locale/num_put/put)

I do wonder why 1024000 would return as 1e+03 KiB instead of 1000 KiB though, so I wonder what configuration is needed to produce this issue.

@AI0867
Copy link
Member

AI0867 commented Jun 8, 2019

Ah, I seem to have found the problem: http://cpp.sh/26hia
Further analysis: http://cpp.sh/7ppnb

There is a range of double values [999.5, 1000) that will display as 1000 when printed. Printing these with a precision of 4 will cause 999.5 to be printed instead, but that's less of an issue.

A more robust comparison will fix this (rounding before comparing will do). We could also round all values >= 999 before printing to avoid the whole thing altogether.

AI0867 added a commit to AI0867/wesnoth that referenced this issue Jun 11, 2019
Behaviour for MSVC remains the same (fixed with 1 decimal).
Fix a corner-case where [999.5, 1000) would not trigger 4-digit precision,
but would be rounded to 1000 during printing, resulting in 1e+3.
@AI0867 AI0867 closed this as completed in 4141602 Jun 21, 2019
AI0867 added a commit that referenced this issue Jun 21, 2019
Restore 3-significant-digit si_string (closes #3871)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add-ons Issues with the add-ons client and/or server. Bug Issues involving unexpected behavior. Good first issue Issues deemed adequate for contributors without prior experience to work on. Ready for testing Issues for which a potential fix is available but untested. UI User interface issues, including both back-end and front-end issues.
Projects
None yet
Development

No branches or pull requests

5 participants