Re: [Mono-dev] System.dll few patches for review
Care to explain why? I'm aware that Invariant will be (invariant-culture) sensitive, however it IMHO would fix the bug. I also agree that using String.Compare with OrdinalIgnoreCase would be the best option, however unfortunatelly that is .Net 2.0 only. String.CompareOrdinal() is obviously obviously a no-go because it would be case-sensitive and as I already wrote CompareOptions are 2.0 only. Happy hacking Andreas P.S. If I'd design a new .Net Framework the first thing would be to create two Classes: (System)String and LanguageString. You are still wrong :-) InvariantCulture still causes culture sensitive comparison. Use String.CompareOrdinal() or CompareOptions.Ordinal instead. Atsushi Eno Andreas Nahr wrote: Sorry if this is already handled, just looked over the list and found this bug: Index: System.Net/DigestClient.cs === --- System.Net/DigestClient.cs (revision 66034) +++ System.Net/DigestClient.cs (working copy) @@ -248,9 +248,9 @@ return false; } - // build the hash object (only MD5 is defined in RFC2617) - if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) - hash = HashAlgorithm.Create (MD5); + // build the hash object (only MD5 is defined in RFC2617) + if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) + hash = MD5.Create (); return true; } Algorithm.ToUpper ().StartsWith (MD5))) is most likely wrong because the code is doing a culture-sensitive uppercasing. You should use something like ToUpperInvariant or pass the Invariant Culture (or better if possible: one of the case-insensitive compares) I didn't look into the relevant classes, but there may be more similar occurences of that problem. mfg Andreas ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Because there are characters that are ignored even in invariant comparison. Well, I agree that instead was not proper here. It should be additionally. Atsushi Eno Andreas Nahr wrote: Care to explain why? I'm aware that Invariant will be (invariant-culture) sensitive, however it IMHO would fix the bug. I also agree that using String.Compare with OrdinalIgnoreCase would be the best option, however unfortunatelly that is .Net 2.0 only. String.CompareOrdinal() is obviously obviously a no-go because it would be case-sensitive and as I already wrote CompareOptions are 2.0 only. Happy hacking Andreas P.S. If I'd design a new .Net Framework the first thing would be to create two Classes: (System)String and LanguageString. You are still wrong :-) InvariantCulture still causes culture sensitive comparison. Use String.CompareOrdinal() or CompareOptions.Ordinal instead. Atsushi Eno Andreas Nahr wrote: Sorry if this is already handled, just looked over the list and found this bug: Index: System.Net/DigestClient.cs === --- System.Net/DigestClient.cs (revision 66034) +++ System.Net/DigestClient.cs (working copy) @@ -248,9 +248,9 @@ return false; } - // build the hash object (only MD5 is defined in RFC2617) - if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) - hash = HashAlgorithm.Create (MD5); + // build the hash object (only MD5 is defined in RFC2617) + if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) + hash = MD5.Create (); return true; } Algorithm.ToUpper ().StartsWith (MD5))) is most likely wrong because the code is doing a culture-sensitive uppercasing. You should use something like ToUpperInvariant or pass the Invariant Culture (or better if possible: one of the case-insensitive compares) I didn't look into the relevant classes, but there may be more similar occurences of that problem. mfg Andreas ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Hello Sebastien. I ran mono-api-check on both 1.1 and 2.0 profile, and it did not see any difference after I removed IEnumerable. So I will commit this patch and check class statuses of 1.1 and 2.0 in a few days. * X509CertificateCollection.patch - remove unnecessary overload If this doesn't cause any error with the class library status pages then remove (don't comment) it. The comment itself can be put in the ChangeLog. According to MSDN X509CertificateCollection does not implement IEnumerable privately. You're right. Anyway CollectionBase already implements IEnumerable. Not sure why it was put there (it's been there since the first commit in 2002). Actually, that means that the patch should look like in the new attachment. What do you mean by class library status pages? The old corcompare which is available online from http://www.mono-project.com/Class_Status You should check both 1.x and 2.0 profiles. I could run make run-test after this patch applied, and it gave same number of errors before and after the patch. Is that enough? No. It will spot functionality regressions but it won't spot errors in API definitions. I don't see how/why this could break but it's safer to always check the pages after an API change (either manually on your own computer or on the public pages a while after the check-in). -- Sebastien Pouliot [EMAIL PROTECTED] Blog: http://pages.infinit.net/ctech/ ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Hello Andrew, Thanks for spotting/fixing this! On Thu, 2006-10-05 at 00:19 -0700, Andrew Skiba wrote: Hello Sebastien. I ran mono-api-check on both 1.1 and 2.0 profile, and it did not see any difference after I removed IEnumerable. So I will commit this patch and check class statuses of 1.1 and 2.0 in a few days. * X509CertificateCollection.patch - remove unnecessary overload If this doesn't cause any error with the class library status pages then remove (don't comment) it. The comment itself can be put in the ChangeLog. According to MSDN X509CertificateCollection does not implement IEnumerable privately. You're right. Anyway CollectionBase already implements IEnumerable. Not sure why it was put there (it's been there since the first commit in 2002). Actually, that means that the patch should look like in the new attachment. What do you mean by class library status pages? The old corcompare which is available online from http://www.mono-project.com/Class_Status You should check both 1.x and 2.0 profiles. I could run make run-test after this patch applied, and it gave same number of errors before and after the patch. Is that enough? No. It will spot functionality regressions but it won't spot errors in API definitions. I don't see how/why this could break but it's safer to always check the pages after an API change (either manually on your own computer or on the public pages a while after the check-in). -- Sebastien Pouliot [EMAIL PROTECTED] Blog: http://pages.infinit.net/ctech/ ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Sorry if this is already handled, just looked over the list and found this bug: Index: System.Net/DigestClient.cs === --- System.Net/DigestClient.cs (revision 66034) +++ System.Net/DigestClient.cs (working copy) @@ -248,9 +248,9 @@ return false; } - // build the hash object (only MD5 is defined in RFC2617) - if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) - hash = HashAlgorithm.Create (MD5); + // build the hash object (only MD5 is defined in RFC2617) + if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) + hash = MD5.Create (); return true; } Algorithm.ToUpper ().StartsWith (MD5))) is most likely wrong because the code is doing a culture-sensitive uppercasing. You should use something like ToUpperInvariant or pass the Invariant Culture (or better if possible: one of the case-insensitive compares) I didn't look into the relevant classes, but there may be more similar occurences of that problem. mfg Andreas ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
You are still wrong :-) InvariantCulture still causes culture sensitive comparison. Use String.CompareOrdinal() or CompareOptions.Ordinal instead. Atsushi Eno Andreas Nahr wrote: Sorry if this is already handled, just looked over the list and found this bug: Index: System.Net/DigestClient.cs === --- System.Net/DigestClient.cs (revision 66034) +++ System.Net/DigestClient.cs (working copy) @@ -248,9 +248,9 @@ return false; } - // build the hash object (only MD5 is defined in RFC2617) - if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) - hash = HashAlgorithm.Create (MD5); + // build the hash object (only MD5 is defined in RFC2617) + if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) + hash = MD5.Create (); return true; } Algorithm.ToUpper ().StartsWith (MD5))) is most likely wrong because the code is doing a culture-sensitive uppercasing. You should use something like ToUpperInvariant or pass the Invariant Culture (or better if possible: one of the case-insensitive compares) I didn't look into the relevant classes, but there may be more similar occurences of that problem. mfg Andreas ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Hello Andrew, On Tue, 2006-10-03 at 10:28 -0700, Andrew Skiba wrote: Hello Sebastien, Part of them is needed to omit TARGET_JVM, so code will be common. There are no TARGET_JVM in the two files. As I said, these changes are needed to omit TARGET_JVM. oops * DigestClient.patch - use MD5.Create instead of HashAlgorithm.Create (MD5) This is ok in the sense that it will result in the exact same thing. So the question is: Is that change required ? if so, can you say why (and include the rational in the ChangeLog). After futher investigation, I see that this old limitation in Grasshopper is gone, so this patch is not necessary anymore. k * X509CertificateCollection.patch - remove unnecessary overload If this doesn't cause any error with the class library status pages then remove (don't comment) it. The comment itself can be put in the ChangeLog. According to MSDN X509CertificateCollection does not implement IEnumerable privately. You're right. Anyway CollectionBase already implements IEnumerable. Not sure why it was put there (it's been there since the first commit in 2002). Actually, that means that the patch should look like in the new attachment. What do you mean by class library status pages? The old corcompare which is available online from http://www.mono-project.com/Class_Status You should check both 1.x and 2.0 profiles. I could run make run-test after this patch applied, and it gave same number of errors before and after the patch. Is that enough? No. It will spot functionality regressions but it won't spot errors in API definitions. I don't see how/why this could break but it's safer to always check the pages after an API change (either manually on your own computer or on the public pages a while after the check-in). -- Sebastien Pouliot [EMAIL PROTECTED] Blog: http://pages.infinit.net/ctech/ ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Hi, Andrew Skiba [EMAIL PROTECTED] writes: Please review these patches for System.dll. Part of them is needed to omit TARGET_JVM, so code will be common. * DigestClient.patch - use MD5.Create instead of HashAlgorithm.Create (MD5) * X509CertificateCollection.patch - remove unnecessary overload Thank you. Andrew. Index: System.Net/DigestClient.cs === --- System.Net/DigestClient.cs(revision 66034) +++ System.Net/DigestClient.cs(working copy) @@ -248,9 +248,9 @@ return false; } - // build the hash object (only MD5 is defined in RFC2617) - if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) - hash = HashAlgorithm.Create (MD5); + // build the hash object (only MD5 is defined in RFC2617) + if ((parser.Algorithm == null) || (parser.Algorithm.ToUpper ().StartsWith (MD5))) + hash = MD5.Create (); return true; } Hmm, there's a stealth CR-LF in the patch. Can you fix that. Even better, set the svn:eol-style property of that file to native to prevent such issues. - Hari ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] System.dll few patches for review
Hello Sebastien, Part of them is needed to omit TARGET_JVM, so code will be common. There are no TARGET_JVM in the two files. As I said, these changes are needed to omit TARGET_JVM. * DigestClient.patch - use MD5.Create instead of HashAlgorithm.Create (MD5) This is ok in the sense that it will result in the exact same thing. So the question is: Is that change required ? if so, can you say why (and include the rational in the ChangeLog). After futher investigation, I see that this old limitation in Grasshopper is gone, so this patch is not necessary anymore. * X509CertificateCollection.patch - remove unnecessary overload If this doesn't cause any error with the class library status pages then remove (don't comment) it. The comment itself can be put in the ChangeLog. According to MSDN X509CertificateCollection does not implement IEnumerable privately. Actually, that means that the patch should look like in the new attachment. What do you mean by class library status pages? I could run make run-test after this patch applied, and it gave same number of errors before and after the patch. Is that enough? Thank you. Andrew. X509CertificateCollection.patch Description: X509CertificateCollection.patch ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
