[Mono-devel-list] Re: XSLT patches reviewed

[Andrew Skiba]

Atsushi Eno wrote:
> Hi,
> 
> So I've largely finished applying the patch. I still have some
> pending stuff:
> 
> xslnumber.patch
> 	I need more precise understanding about number formatting
> 	which is expected in XSLT spec (or please point the related
> 	spec section number in your patch).
The idea here was to pass test which checks large number for grouping
sizes. The standard does not limit grouping sizes to MAX_INT, so to
parse numbers like 10000000000000000 you can use decimal type.

> xsforeach.patch
> 	I need clarification on the purpose of this patch.
> 	Performance?
No, null reference bug. XSLT standard does not forbid you to write
<xsl:for-each /> You can see that children is null in this case, so
children.Evaluate () will crash.

> 
> xsltcompiledcontext.patch
> 	I need clarification. What if there are foo-bar and foo_bar?
I don't remember why this was needed. Let me check this one.

> xslstylesheet.patch
> locationpathpattern.patch
> 	I need clarification on the purpose of these patches.
> 	With OASIS's stable archive I could not find any improvements.

May be the tests that are improved by this patch are noised by minor
differences. I wrote XmlNormalizer which allows you to cut different
nodes from XML, so you can see more important differences and ignore
less important. Have the source in the attachment.

xslstylesheet.patch fixes the bug if you redefine namespace-alias a few
times for a same identifier. You don't want to create a new namespace
alias, so you replace the previous one.

locationpathpattern.patch fixes a bug that was triggered not by this
test suite, but by some Serializer as best as I can remember. At least
in one of the cases when you give '*' it becomes an empty string, so
this check was necessary.

> 
> xmlwriteremitter.patch
> 	I should reject this patch. It is enough to invoke
> 	String.Replace() just once (because it replaces all
> 	the matching string occurence). Also, two WriteComment()
> 	without object creation is better than allocating another
> 	string object in general.
This patch was fixes Comment_Comment_LineOfAllHyphens test. If it was
enough to call String.Replace() just once the test would succed from the
beginning.

> 
> parser.patch
> 	It actually broke the build. We already have ErrorOutput
> 	maybe for the same purpose.
Hm, this one wasn't my change. May be it's not neccessary.

> 
> whitespacenodes.patch
> 	Let me read the spec in depth. Without related spec section
> 	it is impossible to determine if the patch is fine (and that
> 	part is one of the difficult part of XSLT).
This patch fixes null reference exception. It does not change whitespace
outputting. Exception happens with test IDs BVTs_bvt083 and BVTs_bvt001.

> For now, am going to make our standalone tests runnable with the
> new archive and/or reuse your test runner. I prefer mine only
> because there are several options, but in the meantime it is easier
> to reuse Class1.cs.
This class was not a part of any our test suite, I wrote quick and dirty
to start with something. So you will surely need to tune it to be
somewhat useful.

> I believe all the rest part of the patch is in svn, but tell me if
> there was missing. In general very nice patches :-)
Big pleasure to hear it.

Thank you.
Andrew Skiba.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: XmlNormalizer.cs
Url: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20050209/55afa797/attachment.pl