Need to implement the format-number() XSLT function

VERIFIED FIXED in mozilla0.9.6

Status

()

P5
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: peterv, Assigned: sicking)

Tracking

Trunk
mozilla0.9.6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(17 attachments, 1 obsolete attachment)

25.11 KB, patch
Details | Diff | Splinter Review
10.50 KB, text/plain
Details
26.70 KB, patch
Details | Diff | Splinter Review
10.60 KB, text/plain
Details
29.16 KB, patch
Details | Diff | Splinter Review
10.56 KB, text/plain
Details
31.14 KB, patch
Details | Diff | Splinter Review
10.74 KB, patch
Details | Diff | Splinter Review
22.64 KB, patch
Details | Diff | Splinter Review
10.13 KB, text/plain
Details
504 bytes, text/xml
Details
23.12 KB, patch
Details | Diff | Splinter Review
12.80 KB, text/plain
Details
37.38 KB, patch
Details | Diff | Splinter Review
38.33 KB, patch
Details | Diff | Splinter Review
39.17 KB, patch
axel
: review+
Details | Diff | Splinter Review
1.40 KB, text/xml
Details
(Reporter)

Description

18 years ago
We need to implement the format-number() XSLT extension function
(http://www.w3.org/TR/xslt.html#function-format-number) in Transformiix.
(Reporter)

Updated

18 years ago
Blocks: 63906
(Reporter)

Updated

18 years ago
Target Milestone: --- → mozilla0.8
would it be ok if I took over this bug?

Comment 2

18 years ago
Here you go.

We're eager to review, harharhar ;-)

Axel
Assignee: peterv → sicking
Status: NEW → ASSIGNED
I'll have a patch ready in notime :)
can anybody see any use for # character in the integer part of an expression? 
I.e. what number would be differently formatted for "###000" than "000"?

Comment 5

18 years ago
10000, I guess. "##000" should render it as 10000, "000" as NaN, IMHO.
I didn't find any overflow or error handling specs at the java docs, anybody
else with more luck?
Added URL to DecimalFormat docs.

Axel
I found the answer myself...

http://java.sun.com/products/jdk/1.1/docs/api/java.text.DecimalFormat.html#apply
Pattern(java.lang.String)

says that #,#00 means a minimum of 2 integer digits. I think the reason for 
having # in the integer part is that it enables you to specify the grouping 
size without adding extra integer digits...

Hopefully I will have a patch coming up tonight
Created attachment 23990 [details] [diff] [review]
first version of implementation
ok this is my first version of implementation of format-number()

there are however some things I'm not satisfied with:

1. Error handling
- What should format-number return if it's given an errorous format string or a 
nonexisting <decimal-format> name?
- Is there any type of error that are critical and requires the processing to 
halt (I seem to rember something like this in the spec but can't find it)

2. Code size
- The code for adding a <decimal-format> is way too big, mainly due to 
errorchecking. Should the amount of errorchecking be decreased to trim the code?
- Codesize for the FormatNumberFunctionCall could be decreased somewhat aswell

3. DecimalFormat storage
- I'm not 100% happy with how the DecimalFormat structs are stored (in a List). 
I'd like to store them in some sort of NamedList but that seems to require that 
the DecimalFormat implements TxObject which seems unneccesary for struct 
without any functionallity...
- The default values of the DecimalFormat exists in two places in the code...

Any ideas are welcome :)

Comment 9

18 years ago
Hi sicking,
we need your implementation file, too. New files are not part of diffs, you 
have to attach them separatly.
As to error catching and code size:
What about adding the default values to the definition of the struct?
I haven't found anything error handling in the spec, so we might want to issue
a message, but I guess it's safe to continue with default values.

Axel
Created attachment 24084 [details]
actual FormatNumberFunctionCall.cpp file (version 1)
Created attachment 24489 [details]
FormatNumberFunctionCall.cpp version 2
attached is the new (and hopefully final) version of the implementation. The 
only thing I'm still not satisfied with is the fact that the defaultvalues 
appear in two places.

please review and check in :-)
Keywords: review

Comment 14

18 years ago
oh, bad me.
Looking at the code, I have a few comments that should have gone in the first
review:

Make the DecimalFormat a class with a constructor. That's more along the lines.

There is no license header in FormatNumberFunctionCall.cpp, you're the author,
nobody can add the license but you.

Move the attribute checking in ProcessorState::addDecimalFormat to a iteration
over the attributes and a switch statement for the setting, and a switch
statement for the error handling.

You check for <0.0, but there is a negative 0.

I haven't understood defaultDecimalFormatSet, hrm. Could you explain that?

So much for this one, I didn't check the code for functionality yet ;-).

Sorry that I didn't look that closely the first time.

Axel

Comment 15

18 years ago
inQuote should be a MBool, and go for some switch statements in the 
format-number() implementation, too.

Axel, 0.0.3, yac
The reason for defaultDecimalFormatSet is this:

I need to add a default nameless DecimalFormat when no nameless format is given 
(a default default DecimalFormat :-). There are two places when I can do this.

1. After the Top level elements has been processed. The problem with is there 
is currently no function that is called between the toplevel elements has been 
processed and the document buidling starts.

2. Before the Top level elements is processed. The problem with this is when a 
nameless format is added. I need to know if the existing nameless format is the 
default one or if it has been explicitly set by a previous nameless 
<xsl:decimal-format>.

I choose 2 and thus I needed defaultDecimalFormatSet to track if the existing 
nameless decimal format is the default one...
Created attachment 24714 [details]
FormatNumberFunctionCall.cpp version 3
ok here goes version 3

I'm not really sure that using switches in ProcessorState::addDecimalFormat was 
such a good idea since it increased the codesize and IMHO didn't improve 
readability...

Also I'm not sure how to check for -0. Neither <0 and fabs(x) != x works, see 
suggested solution in bug 53518.
hmm... should really -0 be formatted as a negative number? the purpose of 
format-number is to style for a human to read, not to do any calculations on...
(Reporter)

Comment 21

18 years ago
Moving forward.
Target Milestone: mozilla0.8 → mozilla0.9

Comment 22

18 years ago
pushing back, still no happy with the code.
Peter will ask WG about the quoting with "'". Is it quoting, or is it escaping?

Do we want to have '0'0'0, or '000' to get zeros (and other special chars) into
the output. Can't really test this against my java impl. That's horked :-(.
How to get a "'" in the output? escaping would make it '', but the java
implementation suggests it's quoting style.
Both the XSLT and the java spec aren't really precise in what they say or intend
waiting for answers here. Sorry, sicking.

Axel
Hmm.. I should check for QName validness aswell
(Reporter)

Comment 24

18 years ago
Two small notes: in ProcessorState::addDecimalFormat, you should return early
out of the for loop if !success. No need to keep running if we know the source
is wrong. It might clean up the code if you declare an equal operator on
DecimalFormat. Still need to figure out/ask about the quote/escape behaviour.
the patch fails on almost all xalantests because the xalantests use invalid 
formatstrings. Almost all testcases have formatstrings like

###.###

which is invalid because the javaspec says that atleast one '0' is required 
before the decimal separator. 

So the question is, should we accept formats like the above or follow the spec 
strictly? I'd say we should act strictly, though it would be nice to be able to 
format numbers like ".4711"
Created attachment 28938 [details] [diff] [review]
FormatNumberFunctionCall.cpp version 4
the new version does rounding correct as well as supports a special 
XALAN_COMPABILITY so that transformiix behaves a bit more like xalan expects it 
to (although it's wrong according to spec). It still dosn't pass all the xalan 
testcases since xalan seems to have screwed up negative-number-format handling.

Comment 29

18 years ago
I don't think we should care so much about Xalan compability, especially if they 
are incorrect.
This isn't quite ready for review yet. Needs precision love
Keywords: review
need persition fu to fix this, should be able to do that once Double::toString
has better persition.

pushing to 0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
pushing, but now I got a plan at least...
Target Milestone: mozilla0.9.1 → mozilla0.9.2
pushing, will hopefully have this ready real soon
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Updated

18 years ago
No longer blocks: 63906
pushing
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Updated

17 years ago
Priority: -- → P5
pushing
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Created attachment 48006 [details]
txFormatNumberFunctionCall.cpp ver 5
woohoo, finally got this one done!

Adressed all comments that I could remember though this one is quite old so I 
proboly forgot a few...
Keywords: review

Comment 39

17 years ago
Hrm. Doesn't build on unix.
names.h -> Names.h, txDecimalFormat::isEqual needs MBool return type in 
XSLTFunctions.h

I'd think you could init the strings with the maximum size.

in the IsNeg chunk starting at line 107, shouldn't you negate value when
having just one pattern?

l 255 shouldn't need to take the fabs, then.

I'm not sure what the effect of zero-digit is supposed to be when it comes to
output. Xalan says, use it, xt just says that pattern is invalid :-(, 
Peter, could you poke the WG?

I'm somewhat unsure about what is supposed to happen for long formats and
not exact floats. See the upcoming testcase (apply it against a 
<?xml version="1.0"?>
<doc>
</doc>
)
(xt says all three should result in 2.3)
WG?

So much for this bunch.

Axel
Keywords: helpwanted

Comment 40

17 years ago
Created attachment 48082 [details]
formating 2.3 with format of different suffix length
Created attachment 51018 [details]
txFormatNumberFunctionCall.cpp ver 6
ok, this one uses PR_dtoa for module and sprintf for standalone. Unfortuantly 
sprintf behaves about the same as the previous code, but I guess we'll have to 
live with that

Comment 44

17 years ago
patch:
the comment line for ProcessorState::addDecimalFormat is way too long.
don't add addInclude to ProcessorState.h
enum FormatParseState is line too long
impl:
(format == NULL) should be (!format)
remove some ' ' in sums and %, if they are part of a logical statement. That
makes the order of evaluation more obvious.
the file needs the new license plate.

Once this is done, I hope I can compile and test it. (ProcessorState.h blocks
that)

Axel
this missed the train
I said "this missed the train" damnit!
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Comment 49

17 years ago
Hrm. Compile errors on standalone, you need to include stdio.h and
minIntegerSize vs. minItegerSize in the sprintf line.
Please handle Finished in the switch, that gets you a warning less.

For module, I get an output like this when trying to format-number 0.1 thru 0.9:
v.1
?.2
?.30000000000000004
2.3
?.3
?.4
?.5
/.6000000000000001
6.7000000000000001

.9
You miss .8? It looks ok, but I can't paste the result. The 2.3 result looks
good though. Gonna attach the other stylesheet, so we can cross check this.

Axel

Updated

17 years ago
Keywords: helpwanted

Comment 50

17 years ago
Created attachment 53102 [details]
format the number 0.1 thru 0.9
there were defenetly some big problems with numbers starting with 0. Sorry for 
the trouble (I guess i'm getting a bit fed up with this patch :( )

Comment 53

17 years ago
Comment on attachment 53102 [details]
format the number 0.1 thru 0.9

I suck
Attachment #53102 - Attachment is obsolete: true

Comment 54

17 years ago
Created attachment 53240 [details]
format-number() 0.1 thru 0.9
we still don't do all numbers perfectly (i get 0.3 outputted as 
0.3000000000000001), however this seems to be becuase of non-exact
string->double conversion so the double that format-number starts working with 
already has some roundingerrors. However the new string->double code in bug 
96143 makes module work perfectly

Comment 56

17 years ago
Comment on attachment 53212 [details] [diff] [review]
I suck!

    int intDigits = bufIntDigits > minIntegerSize ? bufIntDigits : minIntegerSize;
could use another line, other than that, r=axel@pike.org
(with exactness love)
Attachment #53212 - Flags: review+
+    int intDigits = bufIntDigits > minIntegerSize ? bufIntDigits : minIntege...

is now

+    int intDigits;
+    intDigits = bufIntDigits > minIntegerSize ? bufIntDigits : minIntegerSize;
Comment on attachment 53212 [details] [diff] [review]
I suck!

Fix the 3-space indentation in the switch statement in
the while loop in txFormatNumberFunctionCall::evaluate()

sr=jst
fixed!!! And it only took me 9.5 months :-)

Thanks for reviews!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 60

17 years ago
bitching buttons, verfication spam
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.