logging-log4net-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dangling Pointer <danglingpoin...@outlook.com>
Subject Re: String Equality Comparison, Broken Tests and .NET-1.x
Date Thu, 25 Aug 2016 22:36:08 GMT
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 <dpsenner@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 <danglingpointer@outlook.com<mailto:danglingpointer@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<mailto:Jonas.Baehr@rohde-schwarz.com> <Jonas.Baehr@rohde-schwarz.com<mailto:Jonas.Baehr@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 <bodewig@apache.org<mailto:bodewig@apache.org>> wrote on 23.08.2016
06:14:32:

> Von: Stefan Bodewig <bodewig@apache.org<mailto:bodewig@apache.org>>
> An: "Log4Net Developers List" <log4net-dev@logging.apache.org<mailto: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.Baehr@rohde-schwarz.com<mailto:Jonas.Baehr@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
Mime
View raw message