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

[Atsushi Eno]

Hi,

>>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.

Ok. It is implementation dependent, but I'll commit because it
differentiates behavior between MS and mono. It is now in svn.

>>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.

I see. Now this fix is in svn.

>>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.

OK. Maybe I had better incorporate this normalizer with the test
runner. Let me check again after the work has done.

> 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.

OK, so MS implementation had chosen to recover from the error, in the
way specified in the spec 7.7.1. This fix is now in svn.

> 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.

Yes, looks so, as long as I saw XPath Tokenizer. This fix is now in svn.

>>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.

My first comment for .Replace() was invalid. I'll checkin the first fix.

>>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.

It broke Variables_GlobalVarHaveLocalVarDefinedWithin.rst, so am
pending this patch yet.

> 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.

It was already useful in the first form ;-) I checked in your test
prerequisists and put a result collection to my web repositry.

>>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.

Thanks again for those patches and followups!

Atsushi Eno