190499 - xpath subtraction produces incorrect results

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Ed Schaller]

User-Agent:       Mozilla/5.0 Galeon/1.2.7 (X11; Linux i686; U;) Gecko/20021226 Debian/1.2.7-6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030121

A xpath expression that subtracts two decimal numbers often produces a very
incorrect result. I can produce this most of the time, but not always. So far I
have seen no pattern to it. 5.93 - 5.89 doesn't work, 5.63 - 5.46 does.

Xalan produces correct results for the same files.

Reproducible: Sometimes

Steps to Reproduce:
1.Load http://www.darkmist.net/~schallee/tmp/test.xml
2.
3.

Actual Results:  
5.93 - 5.89 = 40000000000000036

Expected Results:  
5.93 - 5.89 = 0.04

Comment 1

[Ed Schaller]

Attachment: xsl file

Comment 2

[Ed Schaller]

Attachment: xml file

Comment 3

[Axel Hecht]

confirmed. happens in standalone, too. good that we use nspr all over.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ugh, we suck! How could we have missed this for such a long time. The problem is
outputting numbers between 1 and -1, i.e. numbers starting with one or more
zeros. A similar problem was fixed in the format-number() code some time ago,
but I never thought to look for the same problem in the number-to-string code :(

Anyways, patch comming up that fixes the problem. It also make us output
negative-zero as "0" rather then "-0", the former is what the spec says. (Note
that format-number(-0) should and does still output negative-zero as "-0")

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

Comment 6

[Axel Hecht]

Comment on attachment 112583 [details] [diff] [review]
patch to fix

me says needs work.

From my debugging session, I see good indications that we would output
0.040000000000000036.
Anyway, we should fix this function right, that is, kill the bad string do.
Way to go is, compute the total length, SetCapacity, writable iterator and go.
I wonder if we should use nsAFlatString in the interface, so we could use
nsAFlatString::char_iterator.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMO rewriting nsDouble::toString is out of scope for this bug, but feel free to.

Comment 8

[Axel Hecht]

Attachment: fix string usage, use iterators, don't allocate more buffer than needed.

This patch should fix a bunch of issues Double::toString had.
The allocated buffer was way too big. We used nsAString::Append for each char,
that is a huge bunch of work. I moved the code over to nsAString::iterator,
and mimiced the code path that nsAString::do_AppendFromElement goes, with
precomputing the needed length of the resulting string.
Only bad thing, the use of nsAString::SetLength is discouraged. I'll try to 
get jag to sr due to this.

Comment 9

[Axel Hecht]

Comment on attachment 112697 [details] [diff] [review]
fix string usage, use iterators, don't allocate more buffer than needed.

requesting r, sr.
jag, I need to both convert the char buffer I get from PR_dtoa from char to
PRUnichar and add a '.' inside of it. The only way to not copy the buffer twice
was using SetLength, doing stuff do_AppendFromElement and friends do. Could you
sr that?

Comment 10

[Axel Hecht]

Attachment: self-contained testcase

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

+        *dest++ = '-';

iirc postfix ++ is a lot slower then prefix ++ since it will create a copy of
the iterator

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

+    PR_dtoa(aValue, 0, 0, &intDigits, &sign, &endp, buf, 19);

oh, and if you change the "0, 0" to "2, x" we'll get rounding to x significant
digits as well. I've found x=~14 to be a good number, and since it's better to
play it safe then get one extra digit of exactness i would go for 12 or 13 (i'd
rather output 0.300000000000001 as 0.3 then the other way around)

Comment 13

[jag (Peter Annema)]

sicking: note that |*dest++ = '-';| is not the same thing as |*++dest = '-';|

Comment 14

[jag (Peter Annema)]

Comment on attachment 112697 [details] [diff] [review]
fix string usage, use iterators, don't allocate more buffer than needed.

+    char buf[20]; // Mantissa length is 17, so this is plenty
+    PR_dtoa(aValue, 0, 0, &intDigits, &sign, &endp, buf, 19);

Instead of using these two magic numbers, why not #define or |const int| buflen
and calculate buflen-1?

+	 length += intDigits - length;

So |length = intDigits;|?

I'm okay with this use of SetLength.

Comment 15

[Axel Hecht]

2.2204e-16 is the difference between 1 and the next nearest double.
PR_dtoa uses pretty much this, and the length of the mantissa is just like the one
IE uses. That is, 5.93 - 5.89 is not 0.04 in IE either. We trail with 36, IE with
35, IIRC. Note that the spec requires us to output up to the precision if IEEE, 
which is what PR_dtoa does by default.

I'll change 
*dest++= '0'; 
to 
*dest = '0': ++dest;
though

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112697 [details] [diff] [review]
fix string usage, use iterators, don't allocate more buffer than needed.

could you change 
aDest.BeginWriting(dest).advance(PRInt32(oldlength));
to
aDest.BeginWriting(dest).advance(PRInt32(oldlength)-1);

instead of changing all
*dest++ = x;
to
*dest = x; ++dest; ?

This could (and often will) mean positioning the writing-iterator at position
-1, is that something that the writing-iterator can deal with?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112697 [details] [diff] [review]
fix string usage, use iterators, don't allocate more buffer than needed.

either way, r=sicking with jags changes. You could also put an |else| around
the trailing-zeros-loop

Comment 18

[Axel Hecht]

Attachment: fix comments by reviewers

note, I didn't put the
*dest = '0'; ++dest;
on separate lines, because that made the code completely unreadable. I tried,
it really sucked.

Comment 19

[Axel Hecht]

Comment on attachment 112804 [details] [diff] [review]
fix comments by reviewers

carrying r,sr

Comment 20

[Axel Hecht]

Comment on attachment 112804 [details] [diff] [review]
fix comments by reviewers

this is an xpath/xslt-only change. it fixes a conformance problem with numbers
below 1, plus speed issues.
Testing has been done on numerous testcases and a specialized testcase attached
to the bug.

thanx

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 112804 [details] [diff] [review]
fix comments by reviewers

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 22

[Axel Hecht]

fix is in

Comment 23

[Axel Hecht]

mass verifying