668470 - Comment and clean up MathUtils::convertDoubleToString

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

Attachment: Patch

This is a refactoring and commenting patch: it changes no functionality, all bugs are preserved.  In a couple of spots it will take a little bit of thinking to verify that, but I'm firmly convinced it's true.

This patch sets the stage for some ongoing bug fixing.

Comment 1

[Lars T Hansen]

Comment on attachment 543096 [details] [diff] [review]
Patch

Redirecting review from Steven.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #1)
> Comment on attachment 543096 [details] [diff] [review]
> Patch
> 
> Redirecting review from Steven.

(i'll do the review after i return today.)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 543096 [details] [diff] [review]
Patch

Initial feedback below.  Leaving as R? for now, because I want to do a deeper pass later tonight after I get a chance to do a little background reading.

----

``Why "-1"?'' - should this comment, as an implicit to-do, include an associated Bugzilla ticket for follow-on tracking/discussion?

(I suppose the same argument applies to: ``What if exp10 is already the negative of that?''; the latter comment already appears near other fixme comments with bugzilla tags though, so its not as urgent.)

----

-        // fix bug 121952: must also do rounding for fixed mode

Any idea what bug number this really was?  Do we need to preserve it as a to-do?  (Or is it *explaining* what the next block of code is for, and that is why you are getting rid of it?  Again, I need to do a deeper review pass over this code later tonight.)

Comment 4

[Lars T Hansen]

(In reply to Felix S Klock II from comment #3)

> ``Why "-1"?'' - should this comment, as an implicit to-do, include an
> associated Bugzilla ticket for follow-on tracking/discussion?

Not IMO, since there's nobody discussing it...  Again IMO, bugzilla bugs
for minor TODO items like these are not a clear benefit.  The code is what
it is, and figuring it out is part of the process around fixing all the
a2d and d2a bugs, we do not need to individually discuss these items in
bugzilla until there are good reasons to do so, ie, a change is being proposed.

> -        // fix bug 121952: must also do rounding for fixed mode
> 
> Any idea what bug number this really was?

No.  Flashbugbase presumably, but who can say any more?

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 543096 [details] [diff] [review]
Patch

Okay, the refactoring looks solid, apart from one change I was not able to justify without further attempting to understand the underlying code.

(I validated in a manner inspired by wmaddox, by creating my own patch queue of individually verifiable patches that in total add up to something much like attachment 543096 [details] [diff] [review].)

Here's the one change I have not managed to justify: in the for loop below "// Round the value up.", you have changed the termination condition from "ptr >= buffer" to "ptr >= sentinel"

I.e., with my patch queue the change I question is:
@@ -1105,6 +1105,7 @@ namespace avmplus
                 // Round the value up.
                 
-                for ( char *ptr=s-1 ; ptr >= buffer ; ptr-- ) {
-                    if (*ptr < '0')
+                for ( char* ptr=s-1 ; ptr >= sentinel ; ptr-- ) {
+                    // Skip the decimal point
+                    if (*ptr == '.')
                         continue;
                     

The code has established the invariant
  sentinel == (buffer + int(negative))
so this is the difference of one fewer iteration in the negative case.  I cannot trivially determine if this difference matters; that is why I mention it here.  My R+ is assuming that Lars has a justification for the change (or can see a flaw in my reasoning, or will revert to the prior termination condition before pushing the refactoring).

Comment 6

[Lars T Hansen]

(In reply to Felix S Klock II from comment #5)

> The code has established the invariant
>   sentinel == (buffer + int(negative))
> so this is the difference of one fewer iteration in the negative case.  I
> cannot trivially determine if this difference matters; that is why I mention
> it here.  My R+ is assuming that Lars has a justification for the change (or
> can see a flaw in my reasoning, or will revert to the prior termination
> condition before pushing the refactoring).

Yes, that's subtle.  If you look at the handling of 'negative' at the very beginning you see that if the input value is negative then sentinel will indeed be buffer+1, and (this is the important bit) the first character of the buffer will be uninitialized (alloca memory is not routinely zeroed).  Thus it should not be inspected.

Comment 7

[Tamarin Bot]

changeset: 6545:f45bd9336647
user:      Lars T Hansen <[e-mail]>
summary:   Fix 668470 - Comment and clean up MathUtils::convertDoubleToString (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/f45bd9336647