For this particular case, I think we don't need to specialize code for various
versions; just using the least common denominator and
ToUpperInvariantEmptyIfNull method without those diabolical #ifdefs would
suffice.
We can even provide this custom ToUpperEmptyIfNull as string extension method
because it is a language feature and framework version doesn't matter:
<LangVersion>6</LangVersion> in csproj, then:
internal static string ToUpperInvariantEmptyIfNull (this string input)
{
return input?.ToUpper(CultureInfo.InvariantCulture) ?? string.Empty;
}
Usage strA.ToUpperInvariantEmptyIfNull() == strB.ToUpperInvariantEmptyIfNull()
This only requires the build machine to have Roslyn installed to compile C#6
against whatever version of framework (even .NET1.1).
________________________________
From: Dominik Psenner <[email protected]>
Sent: Friday, August 26, 2016 5:38:37 AM
To: Log4NET Dev
Subject: Re: String Equality Comparison, Broken Tests and .NET-1.x
On 26 Aug 2016 12:36 a.m., "Dangling Pointer"
<[email protected]<mailto:[email protected]>> wrote:
>
> Dominik, looks good. I just quickly typed that code in email compose box.
> Your changes are good enough to get incorporated in code base and to conclude
> this issue IMO.
>
>
> Agree that the more backward compatible the better. I just raised the point
> that if less than 1% of log4net consumers are on net2.0 and lower, then they
> most probably are not updating their code or dependency packages to the
> latest versions either. So basically it's just like you said that the newer
> version may just focus on mainstream audience; net35 and higher.
>
>
> > You would not throw away a good 25 year old rum either, would you? :-)
>
>
> I wouldn't dare. :)
>
> But by analogy if it is C lib, I would just comply with C99 and C11 ISO
> standard and would care less about C89, POSIX'ism etc.
Comparing C with .net these standards, the difference is that the langiage is
still mostly the same but other concepts and the API seamlessly evolves.
Choosing a .net framework version is a choice that can be rethought while the
project evolves.
The thing I do not like about the log4net codebase is the preprocessor stuff.
It makes reading and understanding the code almost impossible. Lately I had the
vision to rip apart log4net into several projects where the specialties of the
.net frameworks are handled with overrides/implementation of common interfaces.
But this wont be possible realize with the current manpower.
>
> ________________________________
> From: Dominik Psenner <[email protected]<mailto:[email protected]>>
> Sent: Thursday, August 25, 2016 7:52:37 PM
>
> To: Log4NET Dev
> Subject: Re: String Equality Comparison, Broken Tests and .NET-1.x
>
> At first glance this will not compile:
>
> public static bool NeutralizeString(string input)
> {
> return string.IsNullOrEmpty(input) &&
> input.ToUpper(CultureInfo.InvariantCulture);
> }
>
> Further, the name of the method does not fit yet the purpose of the code.
> Last but not least, I would advise to make it internal.
>
> internal static string GetStringOrEmptyIfNull(string input)
> if (string.IsNullOrEmpty(input))
> return input.ToUpper(CultureInfo.InvariantCulture;
> else
> return string.Empty;
>
> > PS - awesome that log4net has thus far maintain the compatibility with
> > .NET1.1! but are there still consumers of .NET1.1?
>
> There has been a discussion about this some time ago. Please check the
> mailing list backlog. The outcome was that we are stopping to maintain
> everything that is older than .NET 3.5 (exclusive). If someone wants to have
> it, he must A) compile it from source and B) fix the source if it does no
> longer compile. If the effort is cheap, we will however try to keep it
> compatible because of reasons. Maybe we are just old guys that like good old
> stuff. You would not throw away a good 25 year old rum either, would you? :-)
>
> 2016-08-25 18:59 GMT+02:00 Dangling Pointer
> <[email protected]<mailto:[email protected]>>:
>>
>> > 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: [email protected]<mailto:[email protected]>
>> <[email protected]<mailto:[email protected]>>
>> 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 <[email protected]<mailto:[email protected]>> wrote on
>> 23.08.2016 06:14:32:
>>
>> > Von: Stefan Bodewig <[email protected]<mailto:[email protected]>>
>> > An: "Log4Net Developers List"
>> > <[email protected]<mailto:[email protected]>>
>> > Datum: 23.08.2016 06:14
>> > Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
>> >
>> > On 2016-08-22,
>> > <[email protected]<mailto:[email protected]>>
>> > 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
>>
>
>
>
> --
> Dominik Psenner
