Hi!
On Dec 06, Alexey Kopytov wrote:
> On Wednesday 05 December 2007 19:51:46, Sergei Golubchik wrote:
> >On Dec 04, Alexey Kopytov wrote:
> >> ChangeSet@stripped, 2007-12-04 16:06:52+03:00, kaa@polly.(none) +48 -0
> >> WL #2934 "Make/find library for doing float/double to string
> >> conversions and vice versa" Initial import of the dtoa.c code and custom
> >> wrappers around it to allow its usage from the server code.
> >>
> >> Conversion of FLOAT/DOUBLE values to DECIMAL ones or strings and vice
> >> versa has been significantly reworked. As the new algoritms are more
> >> precise than the older ones, results of such conversions may not always
> >> match those obtained from older server versions. This in turn may break
> >> compatibility for some applications.
> >
> >Limit line length in the comments to 80
>
> OK, fixed.
>
> <rants>
> I personally hate this requirement, because the last thing I want to do when
> writing free text (not code) is to think about formatting. And BK tools are
> too dumb to do it for me or at least let me use my preferred editor. If the
> problem with 80 chars limit is relevant only to emails, can we just fix the
> email trigger instead?
> </rants>
Not with email (at least for me) - email client usually can reformat the
test. At least wrap long lines.
The problem is with the later bk comamnds that examine the history - bk
changes, csettool, etc.
Anyway, it could be helped - just committed a config-gui file to internals
repo, put it in ~/.bk/ and it'll handle wrapping for you.
> >> }
> >> }
> >
> >please fix the indentation here
>
> It's a glitch of our coding style. We don't indent cases in the switch()
> statements, and we don't add any offsets with respect to the previous
> statement for opening/closing braces. So, every line here is on the right
> offset according to our rules, but it looks ugly.
ok.
I thought it was a result of diff -b/patch -l.
> >> --- strings/dtoa.c 07/12/04 16:06:46
> >> +++ strings/dtoa.c 07/12/04 16:06:46
> >> +/** Appears to suffice to not call malloc() in most cases */
> >> +#define DTOA_BUFF_SIZE (420 * sizeof(void *))
> >
> >please make sure that the test suite covers cases when dtoa needs to malloc
>
> I don't know such cases. I think the constant above is empirical. The test
> suite passes with malloc()s replaced with asserts. Testing it on "extreme"
> cases (+/- max/min double values, arbitrary values with DBL_DIG + 2
> significant digits), I was never able to cause both dtoa() and strtod() to
> fall back to malloc(). And I've never had enough time to grok the algorithm
> in the level of details required to build such a test case, though I did get
> a general understanding of it while importing the code.
>
> Bottom line: I believe we can say "Appears to suffice to not call malloc() in
> ALL cases" in the comment above, and assert the condition to call malloc()s,
> but I don't have a proof for it yet.
Okay, just add the above to the comment - something along the lines that
in all your tests you've failed to trigger a malloc, but still don't
have a proof that it's impossible.
> >> + /* Number of digits in the exponent from the 'e' conversion */
> >> + exp_len= 1 + (decpt >= 101 || decpt <= -99) + (decpt >= 11 ||
> decpt <=
> >> -9);
> >
> >you don't take into account the sign of the exponent here
>
> Yes, and I don't need to. We only use this for cases when (decpt <= 0) or
> later in the code, where the sign of the exponent is already taken into
> account.
could you mention this in the comment please ?
> >> + /*
> >> + We want to truncate (len - width) least significant digits after
> the
> >> + decimal point. For this we are calling dtoa with mode=5, passing
> the
> >> + number of significant digits = (len-decpt) - (len-width) =
> width-decpt
> >> + */
> >> + dtoa_free(res, buf, sizeof(buf));
> >> + res= dtoa(x, 5, width - decpt, &decpt, &sign, &end, buf,
> sizeof(buf));
> >> + src= res;
> >
> >could you list cases when dtoa will be called twice ?
>
> Any case when all significant digits returned by dtoa() cannot be placed in
> the specified field width using either of the formats. We do our best to
> minimize such cases by:
>
> - passing to dtoa() the field width as the number of significant digits
>
> - removing the sign of the number early (and decreasing the width before
> passing it to dtoa())
>
> - choosing the proper format to preserve the most number of significant
> digits.
Could you add the above to the comment, together with a couple of
examples ?
> >> + /* Do we have to truncate any digits? */
> >> + if (width < len)
> >> + {
> >> + /* Yes, re-convert with a smaller width */
> >> + dtoa_free(res, buf, sizeof(buf));
> >> + res= dtoa(x, 4, width, &decpt, &sign, &end, buf,
> sizeof(buf));
> >
> >could you list cases when dtoa will be called twice ?
> >are they covered by the test suite ?
>
> Same answers as above.
Same (add a couple of examples, if they're different from the previous
case. explanation is the same, it's enough to refer to it, no need to
repeat it)
> Link to the updated patch: http://lists.mysql.com/commits/39436
Only style and comment changes, no need to review again :)
Ok to push
but I'd appreciate if you'd extend comments as I asked above.
Thanks! Great job. Finally, we got it.
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Developer
/_/ /_/\_, /___/\___\_\___/ MySQL GmbH, Dachauer Str. 37, D-80335 München
<___/ Geschäftsführer: Kaj Arnö - HRB
München 162140
