Closed Bug 65983 Opened 24 years ago Closed 23 years ago

Need to implement the format-number() XSLT function

Categories

(Core :: XSLT, defect, P5)

Product:
Core
Component:
XSLT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: peterv, Assigned: sicking)

References

(
URL
)

Details

Attachments

(17 files, 1 obsolete file)

first version of implementation
25.11 KB, patch
Details | Diff | Splinter Review
actual FormatNumberFunctionCall.cpp file (version 1)
10.50 KB, text/plain
Details
implementation-patch version 2
26.70 KB, patch
Details | Diff | Splinter Review
FormatNumberFunctionCall.cpp version 2
10.60 KB, text/plain
Details
implementation-patch version 3
29.16 KB, patch
Details | Diff | Splinter Review
FormatNumberFunctionCall.cpp version 3
10.56 KB, text/plain
Details
implementation-patch version 4
31.14 KB, patch
Details | Diff | Splinter Review
FormatNumberFunctionCall.cpp version 4
10.74 KB, patch
Details | Diff | Splinter Review
implementation-patch version 5
22.64 KB, patch
Details | Diff | Splinter Review
txFormatNumberFunctionCall.cpp ver 5
10.13 KB, text/plain
Details
formating 2.3 with format of different suffix length
504 bytes, text/xml
Details
implementation-patch version 6
23.12 KB, patch
Details | Diff | Splinter Review
txFormatNumberFunctionCall.cpp ver 6
12.80 KB, text/plain
Details
implementation-patch version 7
37.38 KB, patch
Details | Diff | Splinter Review
synced with tip
38.33 KB, patch
Details | Diff | Splinter Review
I suck!
39.17 KB, patch
axel
: review+
Details | Diff | Splinter Review
format-number() 0.1 thru 0.9
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.
Blocks: 63906
Target Milestone: --- → mozilla0.8
would it be ok if I took over this bug?
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"?
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
URL: http://java.sun.com/products/jdk/1.1/...
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
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 :)

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
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
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
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...
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...
Moving forward.
Target Milestone: mozilla0.8 → mozilla0.9
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
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"
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.
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
No longer blocks: 63906
pushing
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Priority: -- → P5
pushing
Target Milestone: mozilla0.9.4 → mozilla0.9.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
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
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
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
I said "this missed the train" damnit!
Target Milestone: mozilla0.9.5 → mozilla0.9.6
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
Keywords: helpwanted
Attached file format the number 0.1 thru 0.9 (obsolete) — 

    
    

    
  
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 on attachment 53102 [details]
format the number 0.1 thru 0.9

I suck

        Attachment #53102 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

    
  
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 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
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
bitching buttons, verfication spam
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: