RawNumber Improvements / Clang Test Fix / Whitespace Cleanup #589
Conversation
|
CI failed, let me dig into this. |
|
CI is hung on VS builds, but I'm sure they'll be okay. One thing that may be more performant is to avoid the additional branching on |
|
This latest commit is actually the best choice, and it actually improves performance for number parsing slightly by removing the runtime branching when deciding to Push a period ('.') or a scientific exponent ('e') char. This seems to be the cleanest solution and is my final work I'd say. |
…it can add precision. In the context of RapidJSON – especially with its float methods on GenericValue – I think this warning holds no water and should be ignored. Trim whitespace off the end of various lines. Added an additional NumberStream specialization that will always perform a TakePush() even when just Take() is called. This supports RawNumber parsing by pushing onto our StackStream particular parts of the number that currently aren't captured because of full precision double parsing, such as the negative sign, scientific number exponents, etc. RawNumber parsing fails with input streams that don't have copy optimization, such as the BasicIStreamWrapper stream. To work around this, instead do the Transcode copy operation by reading from a UTF8 StringStream instead of the original InputStream. Since the NumberStream downcasts all input Ch into chars, we know we're dealing with UTF8/ASCII compatible stack characters during the Transcoding.
|
I rebased everything into a single commit, to keep the history clean and eliminate evidence of backtracking in the design of the fix. |
|
Hi, thank you for your contribution. |
|
Well the errors only occur when building the Value tests because in those tests you have explicit float literals being passed in to GenericValue. If a user doesn't explicitly use floats, or they explicitly cast floats into doubles, then they'll never see these warnings. If you wanted to avoid warnings even in those cases, you'd have to add some float template function specializations to GenericValue and static_cast those floats into doubles. IMHO, you should let the end user dictate what should be warned or ignored based on their usage. I only ignored those warnings in your unittests project. |
|
I see. If it is only in unit test code it is fine. I will try to check later to see whether the tests can avoid them. |
RawNumber parsing with non-insitu streams had a lot of issues. For starters, only copy optimized streams worked, whereas non-copy optimized streams (such as BasicIStreamWrapper) would not work at all. This is fixed by using a StringStream for the source stream during the encoding phase just before RawNumber is invoked.
For all non-insitu streams, many types of numbers were not parsed into the temporary StackStream properly, particularly negative and scientific numbers. I created a new ConsumePush method that does the same thing as Consume, except it calls TakePush() instead of Take(), to grab such chars as '.', 'e', '-', etc.
My editor cleans up trailing whitespace as well, so a lot of diffs are from removing trailing whitespace, which I think is a good thing. I broke those changes up into their own commits, but I believe they're worthwhile including into this PR.