lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 commented on a change in pull request #347: Move call to debugging flag out of loops calling FixedBitSet.Get/Set
Date Fri, 25 Sep 2020 10:20:56 GMT

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



Mime
View raw message