Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable and modernization part 2 #123

Conversation

erikmav
Copy link
Contributor

@erikmav erikmav commented Mar 12, 2024

Enable nullable annotations for the repo, then update a few files with nullable, convert .NET 1 data structures to strongly typed collections, and perform other modernizations. Plus #region removal per a PR comment elsewhere.

@erikmav
Copy link
Contributor Author

erikmav commented Mar 12, 2024

@FreeAndNil , @fluffynuts - I started a widespread nullable and modernization wave in the code after #119 / #122 , part of which is presented here. If you're willing to accept a series of these, I'm aiming to churn through all the files in the repo to do the same cleanup and optimization. Removing .NET 1.0 ArrayLists and Hashtables in favor of regular and concurrent generics will cut some CPU overhead out of all related code paths.

…h nullable, convert .NET 1 data structures to strongly typed collections, and perform other modernizations
@erikmav erikmav force-pushed the dev/erikmav/nullableAndModernization2 branch from a25f1af to 6e4a4cf Compare March 12, 2024 21:40
Copy link
Contributor

@FreeAndNil FreeAndNil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your work.
I've added some remarks and requested some tests (where they are currently missing).
We need to have those methods covered before making changes.

src/log4net/Core/DefaultRepositorySelector.cs Show resolved Hide resolved
src/log4net/Core/DefaultRepositorySelector.cs Show resolved Hide resolved
src/log4net/Core/CompactRepositorySelector.cs Outdated Show resolved Hide resolved
src/log4net/Core/LevelMap.cs Show resolved Hide resolved
src/log4net/Repository/Hierarchy/Logger.cs Outdated Show resolved Hide resolved
src/log4net/Util/SystemInfo.cs Outdated Show resolved Hide resolved
src/log4net/Util/SystemInfo.cs Outdated Show resolved Hide resolved
src/log4net/Util/SystemInfo.cs Show resolved Hide resolved
src/log4net/Util/SystemInfo.cs Outdated Show resolved Hide resolved
src/log4net/Appender/AdoNetAppender.cs Outdated Show resolved Hide resolved
@FreeAndNil
Copy link
Contributor

@erikmav Thank you very much (especially for adding the tests).
After adding the one missing test (RemoveAppender(IAppender) we are ready to merge.

@FreeAndNil FreeAndNil added this to the 3.0.0 milestone Mar 13, 2024
@FreeAndNil
Copy link
Contributor

@erikmav @fluffynuts I've created issue #124 for those improvements.

@FreeAndNil
Copy link
Contributor

@erikmav did you see my comment regarding the missing test?

@erikmav
Copy link
Contributor Author

erikmav commented Mar 15, 2024

Misunderstood the comment. Working on it.

@FreeAndNil
Copy link
Contributor

Thanks a lot.

@FreeAndNil FreeAndNil merged commit 7bde932 into apache:Feature/111-Dropping-support-for-older-runtimes Mar 15, 2024
@erikmav erikmav deleted the dev/erikmav/nullableAndModernization2 branch March 15, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants