[GitHub] [lucenenet] NightOwl888 commented on a change in pull request #347: Move call to debugging flag out of loops calling FixedBitSet.Get/Set

[GitBox <gi...@apache.org>]

NightOwl888 commented on a change in pull request #347:
URL: https://github.com/apache/lucenenet/pull/347#discussion_r494893175



##########
File path: src/Lucene.Net/Support/Diagnostics/Debugging.cs
##########
@@ -26,6 +26,9 @@ namespace Lucene.Net.Diagnostics
     /// </summary>
     internal static class Debugging
     {
+        // LUCENENET specific - use a lazy wrapper around SystemProperties to avoid the repeated cost of getting the value
+        private static Lazy<bool> _assertsEnabled = new Lazy<bool>(() => SystemProperties.GetPropertyAsBoolean("assert", false));

Review comment:
       Actually, some people used `_assertsEnabled` and other people used `assertsEnabled` for private/internal member variables throughout the project. During the API sweep, they were not normalized as the public/protected variables were, primarily because it isn't as high of a priority. 
   
   > The main focus was to eliminate all of the `PascalCase` member variables, which caused a lot of debugging grief - in some cases a field got called where a property should have or vice versa because of errors during porting. Not to mention all of the extra mental anguish trying to work out what is a property and what is a field while debugging. Not sure why that convention was used, as it isn't a common convention in either Java or .NET. But now all of those fields have been renamed back to `camelCase`.
   
   Currently, there are [tests that use Reflection](https://github.com/apache/lucenenet/blob/ece6bea0a3c98961a77d7060b5615fccefabe725/src/Lucene.Net.Tests.TestFramework/Support/TestApiConsistency.cs) to locate naming and other coding violations, but we probably should aim to use a tool such as Resharper or CodeAnalysis as a replacement at some point. The tool we use should have the ability to make customizations so we can use it to catch Java to .NET conversion errors as well (i.e. `size()` should be converted to `Count` or `Length`, not `Size()` or `Size`).
   
   One convention that is a bit odd and is a special case because of the tendency to make a public `getSomething()` method in Java along with a protected `something` member variable. If we converted this to public `Something` property and protected `something` member variable, we end up with a CLS compliance conflict. As a result, all protected members (whether there was a conflicting property or not) were prefixed with `m_`, since prefixing with `_` is also not CLS compliant. Of course, in .NET it really doesn't make much sense to expose the member variable as protected if there is already a property to access or set it, so we probably could change this at some point as well.
   
   > I started using Resharper recently after I discovered that [JetBrains has a free license program for Apache](https://blog.jetbrains.com/blog/2019/05/30/jetbrains-supports-the-apache-software-foundation/). I used it to change all of the single-lined properties to use expression syntax, but haven't dug down into the settings files yet. No objections if you want to add one as long as the rules don't conflict with our API tests.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org