> Unfortunately, this doesn't work if `a` is allowed to be null.
I made this change in https://github.com/apache/log4net/pull/30. I think we can use: trimmedTargetName?.ToUpperInvariant() in C#6 syntax or the older syntax: string.IsNullOrEmpty(trimmedTargetName) && trimmedTargetName.ToUpperInvariant() to fix this problem. For .NET 1.1 compatibility, we can just use, string.IsNullOrEmpty(trimmedTargetName) && trimmedTargetName.ToUpper(CultureInfo.InvariantCulture); everywhere without branching out with preprocessor directives. Or maybe a helper method: public static bool NeutralizeString(string input) { return string.IsNullOrEmpty(input) && input.ToUpper(CultureInfo.InvariantCulture); } Then use NeutralizeString(strA) == NeutralizeString(strB) without specializing for various versions of framework. PS - awesome that log4net has thus far maintain the compatibility with .NET1.1! but are there still consumers of .NET1.1? Why would they care to update the NuGet package, the next version of log4net, when they don't have time to upgrade their project to newer version of the framework.. just a thought.. :p ________________________________ From: jonas.ba...@rohde-schwarz.com <jonas.ba...@rohde-schwarz.com> Sent: Tuesday, August 23, 2016 1:50:29 PM To: Log4NET Dev Subject: Re: String Equality Comparison, Broken Tests and .NET-1.x Stefan Bodewig <bode...@apache.org> wrote on 23.08.2016 06:14:32: > Von: Stefan Bodewig <bode...@apache.org> > An: "Log4Net Developers List" <log4net-dev@logging.apache.org> > Datum: 23.08.2016 06:14 > Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x > > On 2016-08-22, <jonas.ba...@rohde-schwarz.com> wrote: > > > A recent commit [1] changed, among other things, some string equality > > comparisons from `SomeComparer.Compare(a, "B", IgnoreCase) == 0` to > > `a.ToUpperInvariant() == "B"`, see also [2]. > > > > Unfortunately, this doesn't work if `a` is allowed to be null. Currently a > > lot of log4net.Tests are broken because of such a null reference exception > > in `NewLinePatternConverter.ActivateOptions` (apparently "%newline" is > > quite common in pattern layouts ;-). > > Oh, I'm sorry. I must admit I glanced over the PR and applied it without > running the tests. My fault. > > > For new code I tend to opt for `String.Equals(Option, "DOS", > > StringComparison.OrdinalIgnoreCase)` for a fast, case-insensitive > > comparison with fixed ASCII-only patterns, but static > > `String.Equals(String, String, StringComparison)` is not awailable on > > .NET-1.x [3]. > > This is what the original code before PR #16 looked like, but it doesn't > seem to be available for .NET Core, see the discussion around > https://github.com/apache/log4net/pull/16/ > files#diff-51624ab11a9b3d95cc770de1a4e1bdbc Note quite, it used `string.compare(string, string, bool, CultireInfo) == 0` which is available on .NET-1.x, while `String.Equals(string, string StringComparison)` and `ToUpperInvariant` are not. > > Should we create some helper in SystemInfo that provides null-aware, > > ordinal, casing-agnostic string equality comparison, with some #if's > > .NET-1.x? > > +1 Here you go. The attached patch introduces a `SystemInfo.EqualsIgnoringCase(string, string)`, some unit tests, and fixes `NewLinePatternConverter.ActivateOptions` so that the test suite passes again. Please note that I was only able to test with .NET-4.5.2. I have no .NET-1x around, nor .NET Core (maybe we can even drop this #elif). I used the code for these platforms from previous revisions of NewLinePatternConverter.cs. In addition, I'm not sure if I got all the defines for the #if right. Is there some doc for that? regards, Jonas
