FreeAndNil commented on code in PR #123:
URL: https://github.com/apache/logging-log4net/pull/123#discussion_r1522718935
##########
src/log4net/Repository/Hierarchy/Logger.cs:
##########
@@ -329,12 +306,12 @@ public virtual void RemoveAllAppenders()
/// <see cref="IAppender.Close"/> on the appender removed.
/// </para>
/// </remarks>
- public virtual IAppender RemoveAppender(IAppender appender)
+ public virtual IAppender? RemoveAppender(IAppender? appender)
Review Comment:
Having a test would be good.
##########
src/log4net/Repository/Hierarchy/Logger.cs:
##########
@@ -137,17 +127,15 @@ public virtual Level EffectiveLevel
{
get
{
- for(Logger c = this; c != null; c = c.m_parent)
+ for (Logger? c = this; c is not null; c = c.m_parent)
{
- Level level = c.m_level;
-
- // Casting level to Object for performance, otherwise the overloaded
operator is called
- if ((object)level != null)
+ Level? level = c.Level;
+ if (level is not null)
Review Comment:
```suggestion
if (c.Level is Level level)
```
##########
src/log4net/Core/CompactRepositorySelector.cs:
##########
@@ -18,13 +18,14 @@
#endregion
using System;
-using System.Collections;
+using System.Collections.Concurrent;
+using System.Linq;
using System.Reflection;
-using log4net.Appender;
using log4net.Util;
using log4net.Repository;
+#nullable enable
Review Comment:
Can you delete this file?
This class is no longer needed, after removing support for compact framework.
##########
src/log4net/Core/DefaultRepositorySelector.cs:
##########
@@ -350,73 +328,66 @@ public ILoggerRepository CreateRepository(Assembly
repositoryAssembly, Type repo
/// </remarks>
/// <exception cref="ArgumentNullException"><paramref
name="repositoryName"/> is <see langword="null" />.</exception>
/// <exception cref="LogException"><paramref name="repositoryName"/>
already exists.</exception>
- public ILoggerRepository CreateRepository(string repositoryName, Type
repositoryType)
+ public ILoggerRepository CreateRepository(string repositoryName, Type?
repositoryType)
{
- if (repositoryName == null)
+ if (repositoryName is null)
{
- throw new ArgumentNullException("repositoryName");
+ throw new ArgumentNullException(nameof(repositoryName));
}
// If the type is not set then use the default type
- if (repositoryType == null)
- {
- repositoryType = m_defaultRepositoryType;
- }
+ repositoryType ??= m_defaultRepositoryType;
- lock (this)
+ lock (m_lockObj)
{
- ILoggerRepository rep = null;
+ ILoggerRepository? rep = null;
// First check that the repository does not exist
- rep = m_name2repositoryMap[repositoryName] as ILoggerRepository;
- if (rep != null)
+ if (m_name2repositoryMap.ContainsKey(repositoryName))
{
- throw new LogException("Repository [" + repositoryName + "] is
already defined. Repositories cannot be redefined.");
+ throw new LogException($"Repository [{repositoryName}] is already
defined. Repositories cannot be redefined.");
}
- else
+
+ // Look up an alias before trying to create the new repository
+ if (m_alias2repositoryMap.TryGetValue(repositoryName, out
ILoggerRepository? aliasedRepository))
{
- // Lookup an alias before trying to create the new repository
- ILoggerRepository aliasedRepository =
m_alias2repositoryMap[repositoryName] as ILoggerRepository;
- if (aliasedRepository != null)
- {
- // Found an alias
+ // Found an alias
- // Check repository type
- if (aliasedRepository.GetType() == repositoryType)
- {
- // Repository type is compatible
- LogLog.Debug(declaringType, "Aliasing repository [" +
repositoryName + "] to existing repository [" + aliasedRepository.Name + "]");
- rep = aliasedRepository;
+ // Check repository type
+ if (aliasedRepository.GetType() == repositoryType)
+ {
+ // Repository type is compatible
+ LogLog.Debug(declaringType, $"Aliasing repository
[{repositoryName}] to existing repository [{aliasedRepository.Name}]");
+ rep = aliasedRepository;
- // Store in map
- m_name2repositoryMap[repositoryName] = rep;
- }
- else
- {
- // Invalid repository type for alias
- LogLog.Error(declaringType, "Failed to alias repository [" +
repositoryName + "] to existing repository [" + aliasedRepository.Name + "].
Requested repository type [" + repositoryType.FullName + "] is not compatible
with existing type [" + aliasedRepository.GetType().FullName + "]");
+ // Store in map
+ m_name2repositoryMap[repositoryName] = rep;
Review Comment:
Could you add a test case for this code branch?
At the moment this block is not covered?
##########
src/log4net/Util/SystemInfo.cs:
##########
@@ -665,15 +600,15 @@ public static Type GetTypeFromString(Assembly
relativeAssembly, string typeName,
if (loadedAssemblies != null)
{
- Type fallback = null;
+ Type? fallback = null;
// Search the loaded assemblies for the type
foreach (Assembly assembly in loadedAssemblies)
{
Type t = assembly.GetType(typeName, false, ignoreCase);
- if (t != null)
+ if (t is not null)
Review Comment:
is Type t
##########
src/log4net/Util/SystemInfo.cs:
##########
@@ -970,11 +887,7 @@ public static Boolean EqualsIgnoringCase(String a, String
b)
/// the .NET Compact Framework 1.0.
/// </para>
/// </remarks>
- public static readonly Type[] EmptyTypes = new Type[0];
-
- #endregion Public Static Fields
-
- #region Private Static Fields
+ public static readonly Type[] EmptyTypes = Type.EmptyTypes;
Review Comment:
please remove (was also only for compact framework)
##########
src/log4net/Util/SystemInfo.cs:
##########
@@ -638,22 +576,19 @@ public static Type GetTypeFromString(string typeName,
bool throwOnError, bool ig
/// will be searched for the type.
/// </para>
/// </remarks>
- public static Type GetTypeFromString(Assembly relativeAssembly, string
typeName, bool throwOnError, bool ignoreCase)
+ public static Type? GetTypeFromString(Assembly relativeAssembly, string
typeName, bool throwOnError, bool ignoreCase)
{
// Check if the type name specifies the assembly name
if (typeName.IndexOf(',') == -1)
{
- //LogLog.Debug(declaringType, "SystemInfo: Loading type ["+typeName+"]
from assembly ["+relativeAssembly.FullName+"]");
- // Attempt to lookup the type from the relativeAssembly
+ // Attempt to look up the type from the relativeAssembly
Type type = relativeAssembly.GetType(typeName, false, ignoreCase);
- if (type != null)
+ if (type is not null)
Review Comment:
```suggestion
if (relativeAssembly.GetType(typeName, false, ignoreCase) is Type
type)
```
##########
src/log4net/Util/SystemInfo.cs:
##########
@@ -157,14 +118,14 @@ public static string ConfigurationFileLocation
get
{
#if NETSTANDARD2_0_OR_GREATER
- return SystemInfo.EntryAssemblyLocation + ".config";
+ return EntryAssemblyLocation + ".config";
Review Comment:
please remove the comment for .NET Compact Framework
##########
src/log4net/Core/DefaultRepositorySelector.cs:
##########
@@ -489,35 +453,35 @@ public ILoggerRepository[] GetAllRepositories()
/// </exception>
public void AliasRepository(string repositoryAlias, ILoggerRepository
repositoryTarget)
{
- if (repositoryAlias == null)
+ if (repositoryAlias is null)
Review Comment:
Could you add a unit test? This method is currently not covered.
##########
src/log4net/Util/SystemInfo.cs:
##########
@@ -209,13 +172,7 @@ public static string EntryAssemblyLocation
/// change if the runtime is using fibers.
/// </para>
/// </remarks>
- public static int CurrentThreadId
- {
- get
- {
- return System.Threading.Thread.CurrentThread.ManagedThreadId;
- }
- }
+ public static int CurrentThreadId =>
System.Threading.Thread.CurrentThread.ManagedThreadId;
Review Comment:
Can you please remove the remarks in the xml comments?
They are irrelevant and/or misleading.
##########
src/log4net/Util/SystemInfo.cs:
##########
@@ -514,7 +452,7 @@ public static string AssemblyLocationInfo(Assembly
myAssembly)
/// </remarks>
public static string AssemblyQualifiedName(Type type)
{
- return type.FullName + ", " + type.Assembly.FullName;
+ return $"{type.FullName}, {type.Assembly.FullName}";
Review Comment:
Can you delete this method and replace the only call site with
type.AssemblyQualifiedName?
(also cf legacy)
##########
src/log4net/Core/LevelMap.cs:
##########
@@ -129,23 +121,23 @@ public void Add(string name, int value)
/// Create a new Level and add it to the map
/// </para>
/// </remarks>
- public void Add(string name, int value, string displayName)
+ public void Add(string name, int value, string? displayName)
Review Comment:
Having a test for this would also be good.
##########
src/log4net/Appender/AdoNetAppender.cs:
##########
@@ -723,19 +643,20 @@ protected virtual Type ResolveConnectionType()
{
try
{
- return SystemInfo.GetTypeFromString(ConnectionType, true, false);
+ Type? t = SystemInfo.GetTypeFromString(ConnectionType, true, false);
+ if (t is not null)
Review Comment:
```suggestion
if (SystemInfo.GetTypeFromString(ConnectionType, true, false) is
Type t)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
