Closed Bug 94036 (xsl_number) Opened 23 years ago Closed 22 years ago

Fix xsl:number support

Categories

(Core :: XSLT, defect, P2)

Product:
Core
Component:
XSLT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: nisheeth_mozilla, Assigned: sicking)

References

(
URL
)

Details

Attachments

(1 file, 6 obsolete files)

patch checked in
69.10 KB, patch
Details | Diff | Splinter Review 
Peter, I don't have a testcase.  This is based on the conversation we had last
week about outstanding XSLT bugs.  Please attach testcases if you are aware of
them.  Thanks!

Also, we need more information about what exactly is broken in xsl:number. 
Should we have separate bugs for each problem?  Please make the call.
One thing that I know dosn't work is the "format" attribute.
URL: http://www.w3.org/TR/xslt#number
Priority: -- → P2
Target Milestone: --- → mozilla0.9.9
"the last broken piece in our XSLT support"
Keywords: nsbeta1+
Numbering.cpp has a pattern that goes like this:

String countAttr;
xslNumber->getAttr(txXSLTAtoms::count, kNameSpaceID_None,
                   countAttr);
if (!countAttr.isEmpty()) {
    countPattern = ps->getPattern(xslNumber,
                                  ProcessorState::CountAttr);
    ownsPattern = MB_FALSE;
}

DIE. This gets the Attr twice, and getPattern returns 0 if the attr didn't 
exist. If 0 isn't safe enough for you, I can wholeheartly change the signature
to
nsresult getPattern(Element*, PatternAttr, txPattern*&); in bug 113611.

If you do, please say so, 'cause I will probably touch all those lines anyway.
Target Milestone: mozilla0.9.9 → mozilla1.0
I bumped into this bug, myself. 
http://www.zvon.org/xxl/XSLTutorial/Books/Output/example21_ch8.html illustrates 
the expected behavior of xml:number when used with multilevel values.  Instead 
of outputting in an outline format as shown in the example, xml:number returns 
a sequence along the lines of 1, 11, 12, 2, 21, 22, 23, 3, etc..  It seems to 
be disregarding the format and level attributes, and just going with the 
default.
ADT3 per ADT triage.
Whiteboard: [ADT3]
Status: NEW → ASSIGNED
Keywords: nsbeta1-
OS: Windows 2000 → All
Hardware: PC → All
Whiteboard: [ADT3]
Target Milestone: mozilla1.0 → mozilla1.2alpha
i've come a fair way on the implementation of this. Unfortuantly i've noticed
that the new code isn't much faster then the old one (though of course it
actually implements a good deal more of the spec now). So I've got a question
for ya:

Is the current speed of xsl:number acceptable?

A good way to test the speed is to run the numbering tests in buster. I have
thought of a way that should improve speed, but it isn't trivial, the question
is, is it worth it?
Alias: xsl_number
speed is never enough.
It might be worth implementing the spec first, get some data from the tests
and make a plan for speed then.
This should fully implement xsl:number. There is one thing with the patch that
i'm not fully happy with and that is that it uses global static strings, feel
free to come with suggestions what to do instead.

The patch doesn't implement any i18n counters, that'll have to wait to a later
patch. I havn't found any interfaces for getting the data that we need (though
i havn't looked too hard) so implementing i18n would probably require some
changes on the mozilla side as well
Forgot that i hadn't done the correct thing for numbering documents and
attributes (which is compleatly pointless, but nonetheless allowed)
Attachment #94148 - Attachment is obsolete: true
Since Jonas made the fix he should have this bug on his list, reassigning.
Assignee: peterv → sicking
Status: ASSIGNED → NEW
Keywords: nsbeta1+
Generally, do we need NS_ENSURE_SUCCESS()?

List.h/.cpp:
please make those functions return nsresult instead of MBool.

XSLTProcessor.cpp:
@@ -1499,7 +1499,7 @@
     // xsl:number
     else if (localName == txXSLTAtoms::number) {
         String result;
-        Numbering::doNumbering(actionElement, result, aPs);
+        txXSLTNumber::createNumber(actionElement, aPs, result);
         NS_ASSERTION(mResultHandler, "mResultHandler must not be NULL!");
         mResultHandler->characters(result);
     }
Check the return value.


txXSLTNumber::createNumber()
+    if (NS_FAILED(rv)) {
+        aResult.clear();
+        return rv;
+    }
aResult is already cleared.

txXSLTNumber::getCounters
The code path is a bit funky, if there's no format attr.
I'd at least like to see you not call processAttrValueTemplate

txXSLTNumber::isAlphaNumeric
I wonder if we can share code with XMLUtils here.

I think that the counters in txXSLTNumberCounters.cpp could go into
txXSLTNumber.cpp.

txXSLTNumber::getValueList to come, and txXSLTNumberCounters.cpp too
Status: NEW → ASSIGNED
> Generally, do we need NS_ENSURE_SUCCESS()?
Yes, but IMHO that is a separate bug.


> XSLTProcessor.cpp:
> @@ -1499,7 +1499,7 @@
>      // xsl:number
>      else if (localName == txXSLTAtoms::number) {
>          String result;
> -        Numbering::doNumbering(actionElement, result, aPs);
> +        txXSLTNumber::createNumber(actionElement, aPs, result);
>          NS_ASSERTION(mResultHandler, "mResultHandler must not be NULL!");
>          mResultHandler->characters(result);
>      }
> Check the return value.

And do what? The error should already be reported at this stange, and I can't
propagate the error any further since it's a void function.

> txXSLTNumber::isAlphaNumeric
> I wonder if we can share code with XMLUtils here.

Just looked into this and unfortuantly we can't really share much. The only
thing I could call into from txXSLTNumber::isAlphaNumeric is XMLUtils::isDigit,
which probably gives more pain then the 15 lines of gain.


Re: #12, just add a comment then, so once we fix it, we know that we have to
fix that there.
re NS_ENSURE_SUCCESS, sneaking in that macro is less hassle than doing all those 
if (NS_FAILED(rv)) 
now and fix them later.

Re: #13, so leave that code like it is, bad spec to have all kinds of char defs
around.
txXSLTNumber::getValueList
if value < 0.5, shouldn't you output 0, instead of 
Double::toString(value, aValueString) ?

I wonder if we should use atoms for level, and compare against
txXSLTAtoms::multiple etc. You don't error on level="junk".

Index: source/xslt/txXSLTNumber.h

+class txFormattedCounter;
not needed.

txDecimalCounter::txDecimalCounter
why do you set this mGroupSize to 50?

txDecimalCounter::appendNumber

+    // in case we didn't get a long enough string
+    while (pos > 0 && bufsize - pos < mMinLength) {
+        buf[--pos] = '0';
+    }

make this a for loop.

+    // in case we *still* didn't get a long enough string
+    // this should be very rare
+    PRInt32 extraPos = mMinLength;
+    while (extraPos > bufsize - pos) {
pos is 0 here, and adjust the comment to say that mMinLength is bigger than
the length of any PRInt32, so we have to pad that here.

I'm thru with it.
> Re: #12, just add a comment then, so once we fix it, we know that we have to
> fix that there.
Done


> re NS_ENSURE_SUCCESS, sneaking in that macro is less hassle than doing all thos
> if (NS_FAILED(rv)) 
> now and fix them later.
Done, I added NS_ENSURE_TRUE and NS_ENSURE_FALSE too

> txXSLTNumber::getValueList
> if value < 0.5, shouldn't you output 0, instead of 
> Double::toString(value, aValueString) ?
Nope, look in the errata. I actually don't do exactly as the errata says, but
rather do what XSLT2 says which is sligtly different, it says that the head and
tail of the format should be added even in this case.

> I wonder if we should use atoms for level, and compare against
> txXSLTAtoms::multiple etc. You don't error on level="junk".
Done

> +class txFormattedCounter;
> not needed.
Done

> txDecimalCounter::txDecimalCounter
> why do you set this mGroupSize to 50?
This guarantees that the grouping separator will never be inserted, i.e. we use
a plain decimal counter. This ctor is only used in txRomanCounter::appendNumber
for numbers > 3999 where roman counting isn't defined.


> +    // in case we didn't get a long enough string
> +    while (pos > 0 && bufsize - pos < mMinLength) {
> +        buf[--pos] = '0';
> +    }
> make this a for loop.

The only two for-loops i can make of this is

for (; pos > 0 && bufsize - pos < mMinLength; buf[--pos] = '0');
(which will give a warning)

and

for (; pos > 0 && bufsize - pos < mMinLength; buf[pos] = '0') {
    --pos;
}

which i both think are worse then the while-loop


> +    // in case we *still* didn't get a long enough string
> +    // this should be very rare
> +    PRInt32 extraPos = mMinLength;
> +    while (extraPos > bufsize - pos) {
> pos is 0 here, and adjust the comment to say that mMinLength is bigger than
> the length of any PRInt32, so we have to pad that here.
Done

Addresses Axels comments.

Oh, the reason i split up the counters into thier own file is when we do i18n
we will probably get quite a lot of ugly lengthy code which is nice to keep
separate from everything else.

Also fixed the mGroupSize = 50, Axel is indeed right
Attachment #94150 - Attachment is obsolete: true
Comment on attachment 95376 [details] [diff] [review]
Patch to fully implement xsl:number v2

marking needs work, thought this is much more "needs -N".
Most of the code is missing :-(
Attachment #95376 - Flags: needs-work+
oops, cvs add is good
Attachment #95376 - Attachment is obsolete: true
Comment on attachment 95503 [details] [diff] [review]
Patch to fully implement xsl:number v2

to build this on standalone, I had to switch from private to protected in
TxString.h:188,
and change PRBool to MBool in
txXSLTNumberCounters.cpp:74 and add
#include "txError.h"
to List.h

my version of the loop would be
    PRInt32 end  = (bufsize > mMinLength) ? bufsize - mMinLength : 0;
    while (pos > end) {
vs.
while (pos > 0 && bufsize - pos < mMinLength) {

looking at functionality now
Comment on attachment 95503 [details] [diff] [review]
Patch to fully implement xsl:number v2

r=axel@pike.org, as my comments are addressed.
We had some thoughts about what we should do for empty value lists, as Xalan
seems to take a little freedom and some 0s there. Jonas's patch does what saxon
does, so it's the spec and Jonas and saxon and I vs. Xalan. Sorry buddy. Oh, IE
seems to be on the specs side, too. We'll adjust the xalan tests, I guess.
Attachment #95503 - Flags: review+
Fixes the buildproblems and the while-loop
Attachment #95503 - Attachment is obsolete: true
Comment on attachment 95548 [details] [diff] [review]
Patch to fully implement xsl:number v2.1

carrying over r=Pike
Attachment #95548 - Flags: review+
Fixes a couple of stringchanges peterv requested
Attachment #95548 - Attachment is obsolete: true
Comment on attachment 95898 [details] [diff] [review]
Patch to fully implement xsl:number v2.2

carrying review
Attachment #95898 - Flags: review+
Comment on attachment 95898 [details] [diff] [review]
Patch to fully implement xsl:number v2.2

- In txError.h:

+#define NS_ENSURE_TRUE(value, result) \
+    if (!(value)) {		       \
+	 return (result);	       \
+    }

Please use the PR_BEGIN_MACRO and PR_END_MACRO macro's here to avoid having the
side-effects of the "if" in the macro leak outside the macro. (by doing this an
else after one of those macro's won't do the unexpected and the compiler will
require a ; after those macros).

Other than that, sr=jst
Attachment #95898 - Flags: review+ → superreview+
Comment on attachment 95898 [details] [diff] [review]
Patch to fully implement xsl:number v2.2

- In txError.h:

+#define NS_ENSURE_TRUE(value, result) \
+    if (!(value)) {		       \
+	 return (result);	       \
+    }

Please use the PR_BEGIN_MACRO and PR_END_MACRO macro's here to avoid having the
side-effects of the "if" in the macro leak outside the macro. (by doing this an
else after one of those macro's won't do the unexpected and the compiler will
require a ; after those macros).

Other than that, sr=jst
Attached patch patch checked in — — Splinter Review 
addresses jsts comments and merges to tip
Attachment #95898 - Attachment is obsolete: true
checked in! thanks for reviews
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
mass verifying
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: