Closed
Bug 94036
(xsl_number)
Opened 23 years ago
Closed 22 years ago
Fix xsl:number support
Categories
(Core :: XSLT, defect, P2)
Core
XSLT
Tracking
()
VERIFIED
FIXED
mozilla1.2alpha
People
(Reporter: nisheeth_mozilla, Assigned: sicking)
References
()
Details
Attachments
(1 file, 6 obsolete files)
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.
Assignee | ||
Comment 1•23 years ago
|
||
One thing that I know dosn't work is the "format" attribute.
Updated•23 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla0.9.9
"the last broken piece in our XSLT support"
Keywords: nsbeta1+
Comment 3•23 years ago
|
||
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.
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.0
Comment 4•22 years ago
|
||
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.
Updated•22 years ago
|
Updated•22 years ago
|
Status: NEW → ASSIGNED
Keywords: nsbeta1-
OS: Windows 2000 → All
Hardware: PC → All
Whiteboard: [ADT3]
Target Milestone: mozilla1.0 → mozilla1.2alpha
Assignee | ||
Comment 6•22 years ago
|
||
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
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
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
Assignee | ||
Comment 9•22 years ago
|
||
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.
Comment 11•22 years ago
|
||
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
Assignee | ||
Comment 12•22 years ago
|
||
> 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.
Assignee | ||
Comment 13•22 years ago
|
||
> 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.
Comment 14•22 years ago
|
||
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.
Comment 15•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
> 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
Assignee | ||
Comment 17•22 years ago
|
||
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 18•22 years ago
|
||
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+
Assignee | ||
Comment 19•22 years ago
|
||
oops, cvs add is good
Attachment #95376 -
Attachment is obsolete: true
Comment 20•22 years ago
|
||
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 21•22 years ago
|
||
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+
Assignee | ||
Comment 22•22 years ago
|
||
Fixes the buildproblems and the while-loop
Attachment #95503 -
Attachment is obsolete: true
Assignee | ||
Comment 23•22 years ago
|
||
Comment on attachment 95548 [details] [diff] [review] Patch to fully implement xsl:number v2.1 carrying over r=Pike
Attachment #95548 -
Flags: review+
Assignee | ||
Comment 24•22 years ago
|
||
Fixes a couple of stringchanges peterv requested
Assignee | ||
Updated•22 years ago
|
Attachment #95548 -
Attachment is obsolete: true
Assignee | ||
Comment 25•22 years ago
|
||
Comment on attachment 95898 [details] [diff] [review] Patch to fully implement xsl:number v2.2 carrying review
Attachment #95898 -
Flags: review+
Comment 26•22 years ago
|
||
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 27•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #95898 -
Flags: review+
Assignee | ||
Comment 28•22 years ago
|
||
addresses jsts comments and merges to tip
Attachment #95898 -
Attachment is obsolete: true
Assignee | ||
Comment 29•22 years ago
|
||
checked in! thanks for reviews
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•