You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4net-dev@logging.apache.org by Jo...@rohde-schwarz.com on 2016/08/22 13:55:45 UTC
String Equality Comparison, Broken Tests and .NET-1.x
Hi,
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 ;-).
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].
So
- `a.ToUpperInvariant() == "FOO" is not null-save and produces needless
intermediate strings on the heap (and culture-aware operations are slower
then nesessary for pure ASCII-stuff),
- `SomeComparer.Compare(a, "B", IgnoreCase) == 0` is hard to read, esp.
when the compater is "long".
- we need .NET-1.x support, so all the goodness from [4] does not apply.
Should we create some helper in SystemInfo that provides null-aware,
ordinal, casing-agnostic string equality comparison, with some #if's
.NET-1.x? Who needs .NET-1.x? The current solution with `ToUpperInvariant`
is also .NET-2.0...
[1]: r1756284 ".NET Core improvements by Peter Jas - closes #30"
[2]: https://github.com/apache/log4net/pull/16#discussion_r74683879
[3]:
https://msdn.microsoft.com/en-us/library/t4411bks(v=vs.110).aspx#Anchor_4
[4]: https://msdn.microsoft.com/en-us/library/ms973919.aspx
Regards,
Jonas
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Dominik Psenner <dp...@gmail.com>.
Thanks for your input Jonas. #30 is all around adding support for .net core
and so I believe there might follow a few fixing commits. Anyway, the
current status of .NET 1.x support is that we will drop it anytime soon. It
is so ancient that people might as well stay with the current state of the
framework. Its probably no longer worth the effort to maintain these old
branches. But if it is easy to keep support for it, I would not hesitate to
write a fix. Btw why not rewrite:
a.ToUpperInvariant()
to:
foo = "FOO"
a != null && foo == null || a.ToUpperInvariant() == foo
and there is still the option to #ifdef .net-1.x.
2016-08-22 15:55 GMT+02:00 <Jo...@rohde-schwarz.com>:
> Hi,
>
> 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 ;-).
> 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].
>
> So
> - `a.ToUpperInvariant() == "FOO" is not null-save and produces needless
> intermediate strings on the heap (and culture-aware operations are slower
> then nesessary for pure ASCII-stuff),
> - `SomeComparer.Compare(a, "B", IgnoreCase) == 0` is hard to read, esp.
> when the compater is "long".
> - we need .NET-1.x support, so all the goodness from [4] does not apply.
>
> Should we create some helper in SystemInfo that provides null-aware,
> ordinal, casing-agnostic string equality comparison, with some #if's
> .NET-1.x? Who needs .NET-1.x? The current solution with `ToUpperInvariant`
> is also .NET-2.0...
>
> [1]: r1756284 ".NET Core improvements by Peter Jas - closes #30"
> [2]: https://github.com/apache/log4net/pull/16#discussion_r74683879
> [3]: https://msdn.microsoft.com/en-us/library/t4411bks(v=vs.110).
> aspx#Anchor_4
> [4]: https://msdn.microsoft.com/en-us/library/ms973919.aspx
>
> Regards,
> Jonas
--
Dominik Psenner
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Dangling Pointer <da...@outlook.com>.
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 <dp...@gmail.com>
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" <da...@outlook.com>> 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 <dp...@gmail.com>>
> 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 <da...@outlook.com>>:
>>
>> > 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.Baehr@rohde-schwarz.com<ma...@rohde-schwarz.com> <Jo...@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 <bo...@apache.org>> wrote on 23.08.2016 06:14:32:
>>
>> > Von: Stefan Bodewig <bo...@apache.org>>
>> > An: "Log4Net Developers List" <lo...@logging.apache.org>>
>> > Datum: 23.08.2016 06:14
>> > Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
>> >
>> > On 2016-08-22, <Jo...@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
>>
>
>
>
> --
> Dominik Psenner
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Dominik Psenner <dp...@gmail.com>.
On 26 Aug 2016 12:36 a.m., "Dangling Pointer" <da...@outlook.com>
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 <dp...@gmail.com>
> 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 <da...@outlook.com>:
>>
>> > 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.Baehr@rohde-schwarz.com <Jo...@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 <bo...@apache.org> wrote on 23.08.2016 06:14:32:
>>
>> > Von: Stefan Bodewig <bo...@apache.org>
>> > An: "Log4Net Developers List" <lo...@logging.apache.org>
>> > Datum: 23.08.2016 06:14
>> > Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
>> >
>> > On 2016-08-22, <Jo...@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
>>
>
>
>
> --
> Dominik Psenner
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Dangling Pointer <da...@outlook.com>.
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. [😉]
________________________________
From: Dominik Psenner <dp...@gmail.com>
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 <da...@outlook.com>>:
> 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.Baehr@rohde-schwarz.com<ma...@rohde-schwarz.com> <Jo...@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 <bo...@apache.org>> wrote on 23.08.2016 06:14:32:
> Von: Stefan Bodewig <bo...@apache.org>>
> An: "Log4Net Developers List" <lo...@logging.apache.org>>
> Datum: 23.08.2016 06:14
> Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
>
> On 2016-08-22, <Jo...@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
--
Dominik Psenner
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Dominik Psenner <dp...@gmail.com>.
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 <da...@outlook.com>:
> > 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.Baehr@rohde-schwarz.com <Jo...@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 <bo...@apache.org> wrote on 23.08.2016 06:14:32:
>
> > Von: Stefan Bodewig <bo...@apache.org>
> > An: "Log4Net Developers List" <lo...@logging.apache.org>
> > Datum: 23.08.2016 06:14
> > Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
> >
> > On 2016-08-22, <Jo...@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
>
>
--
Dominik Psenner
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Dangling Pointer <da...@outlook.com>.
> 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.Baehr@rohde-schwarz.com <Jo...@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 <bo...@apache.org> wrote on 23.08.2016 06:14:32:
> Von: Stefan Bodewig <bo...@apache.org>
> An: "Log4Net Developers List" <lo...@logging.apache.org>
> Datum: 23.08.2016 06:14
> Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
>
> On 2016-08-22, <Jo...@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
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Stefan Bodewig <bo...@apache.org>.
Hi Jonas
On 2016-08-23, <Jo...@rohde-schwarz.com> wrote:
> Stefan Bodewig <bo...@apache.org> wrote on 23.08.2016 06:14:32:
>> On 2016-08-22, <Jo...@rohde-schwarz.com> wrote:
>>> 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.
Thanks a lot. I finally managed to apply it. I had to modify it slightly
(a missing using statement and semicolon in the netstandard branch).
Tests should all pass now.
Stefan
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Jo...@rohde-schwarz.com.
Stefan Bodewig <bo...@apache.org> wrote on 23.08.2016 06:14:32:
> Von: Stefan Bodewig <bo...@apache.org>
> An: "Log4Net Developers List" <lo...@logging.apache.org>
> Datum: 23.08.2016 06:14
> Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
>
> On 2016-08-22, <Jo...@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
Re: String Equality Comparison, Broken Tests and .NET-1.x
Posted by Stefan Bodewig <bo...@apache.org>.
On 2016-08-22, <Jo...@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
> 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
Stefan