Closed
Bug 65983
Opened 24 years ago
Closed 23 years ago
Need to implement the format-number() XSLT function
Categories
(Core :: XSLT, defect, P5)
Core
XSLT
Tracking
()
VERIFIED
FIXED
mozilla0.9.6
People
(Reporter: peterv, Assigned: sicking)
References
()
Details
Attachments
(17 files, 1 obsolete file)
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 |
We need to implement the format-number() XSLT extension function (http://www.w3.org/TR/xslt.html#function-format-number) in Transformiix.
Reporter | ||
Updated•24 years ago
|
Target Milestone: --- → mozilla0.8
Assignee | ||
Comment 1•24 years ago
|
||
would it be ok if I took over this bug?
Comment 2•24 years ago
|
||
Here you go. We're eager to review, harharhar ;-) Axel
Assignee: peterv → sicking
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•24 years ago
|
||
I'll have a patch ready in notime :)
Assignee | ||
Comment 4•24 years ago
|
||
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•24 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
Assignee | ||
Comment 6•24 years ago
|
||
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
Assignee | ||
Comment 7•24 years ago
|
||
Assignee | ||
Comment 8•24 years ago
|
||
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•24 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
Assignee | ||
Comment 10•24 years ago
|
||
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
Assignee | ||
Comment 13•24 years ago
|
||
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•24 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•24 years ago
|
||
inQuote should be a MBool, and go for some switch statements in the format-number() implementation, too. Axel, 0.0.3, yac
Assignee | ||
Comment 16•24 years ago
|
||
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...
Assignee | ||
Comment 17•24 years ago
|
||
Assignee | ||
Comment 18•24 years ago
|
||
Assignee | ||
Comment 19•24 years ago
|
||
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.
Assignee | ||
Comment 20•24 years ago
|
||
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...
Comment 22•24 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
Assignee | ||
Comment 23•24 years ago
|
||
Hmm.. I should check for QName validness aswell
Reporter | ||
Comment 24•24 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.
Assignee | ||
Comment 25•24 years ago
|
||
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"
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
Assignee | ||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
I don't think we should care so much about Xalan compability, especially if they are incorrect.
Assignee | ||
Comment 30•23 years ago
|
||
This isn't quite ready for review yet. Needs precision love
Keywords: review
Assignee | ||
Comment 31•23 years ago
|
||
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
Assignee | ||
Comment 32•23 years ago
|
||
pushing, but now I got a plan at least...
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee | ||
Comment 33•23 years ago
|
||
pushing, will hopefully have this ready real soon
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Updated•23 years ago
|
Priority: -- → P5
Assignee | ||
Comment 36•23 years ago
|
||
Assignee | ||
Comment 37•23 years ago
|
||
Assignee | ||
Comment 38•23 years ago
|
||
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•23 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•23 years ago
|
||
Assignee | ||
Comment 41•23 years ago
|
||
Assignee | ||
Comment 42•23 years ago
|
||
Assignee | ||
Comment 43•23 years ago
|
||
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•23 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
Assignee | ||
Comment 45•23 years ago
|
||
Assignee | ||
Comment 46•23 years ago
|
||
Assignee | ||
Comment 47•23 years ago
|
||
this missed the train
Assignee | ||
Comment 48•23 years ago
|
||
I said "this missed the train" damnit!
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment 49•23 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•23 years ago
|
Keywords: helpwanted
Comment 50•23 years ago
|
||
Assignee | ||
Comment 51•23 years ago
|
||
Assignee | ||
Comment 52•23 years ago
|
||
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•23 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•23 years ago
|
||
Assignee | ||
Comment 55•23 years ago
|
||
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•23 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+
Assignee | ||
Comment 57•23 years ago
|
||
+ int intDigits = bufIntDigits > minIntegerSize ? bufIntDigits : minIntege... is now + int intDigits; + intDigits = bufIntDigits > minIntegerSize ? bufIntDigits : minIntegerSize;
Comment 58•23 years ago
|
||
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
Assignee | ||
Comment 59•23 years ago
|
||
fixed!!! And it only took me 9.5 months :-) Thanks for reviews!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•