lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] jeme commented on pull request #373: Remove delegate based debugging assert
Date Thu, 22 Oct 2020 09:11:41 GMT

jeme commented on pull request #373:
URL: https://github.com/apache/lucenenet/pull/373#issuecomment-714351524


   > Hi @jeme, no worries, I also found the final code a bit confusing.
   > 
   > The reason for the redundant check is that I didn't want to change the behavior from:
   > 
   > **Check Flag** ----> **Check if Assert condition is true** ----> **Throw**
   > 
   > To:
   > 
   > **Check if Assert condition is true** ----> **Check Flag** ----> **Throw**
   > 
   > This would happen if we only have the ShouldAssert call - even with the aggressive
inlining option on the ShouldAssert method, the compiler will obviously not invert the checks
as that could change the code behavior.
   > 
   > ```cs
   > if(Debugging.ShouldAssert(*** Some Check ***)){}
   > ```
   > 
   > Thus I added back the Debugging.AssertsEnabled check before the call to ShouldAssert
to keep it consistent with the original code, and to avoid the extra checks due to the condition
on every call even when Debugging.AssertsEnabled = false.
   
   But we have to choose between Simplicity or Performance in this particular case... The
attempt here looks like you went for simplicity first, but then in adding in the performance
after that it completely nullified the simplicity attempt, and even made it much much worse.
   
   So we have to choose...
   ```
   // "Simplicity" (Well somewhat, the Debug.Assert(check, message) is probably the most simple
design)...
   if(Debug.ShouldThrow(*** Check ***)) Debug.Throw("Message");
   ShouldThrow(bool check) => AssertsEnabled && check;
   ```
   If the "Check" is costly, this will hurt performance regardless of Asserts being enabled
or not.
   We do save allocation and computation of the resulting message.
   But I don't think we will go for that.
   
   Or
   ```
   // Performance
   if(Debug.AssertsEnabled && Debug.ShouldThrow(*** Check ***)) Debug.Throw("Message");
   ShouldThrow(bool check) => check;
   ```
   
   In the later you will quickly realize that we are essentially passing a boolean to a method
just to return it, so the method becomes irrelevant all together and only adds noise, hence
we can eliminate that to:
   ```
   // Performance
   if(Debug.AssertsEnabled && *** Check ***) Debug.Throw("Message");
   ```
   This saves the computation of the check, if it's costly that matter, otherwise it is negligible.
   We still save allocation and computation of the resulting message.
   But I don't think we will go for that.
   
   Obviously we can then as a final step simply inline the throw of the exception. I have
no opinion on that here.
   
   ----
   
   The example overloads like @NightOwl888 presented:
   ```
   Assert<T0>(bool condition, string messageFormat, T0 arg0);
   Assert<T0, T1>(bool condition, string messageFormat, T0 arg0, T1 arg1);
   Assert<T0, T1, T2>(bool condition, string messageFormat, T0 arg0, T1 arg1, T2 arg2);
   ```
   Should avoid the extra allocations as well and is the option most in line with what is
currently in place. This is also what is most in line with the built-in "Debug.Assert".
   
   ----
   
   I would as much as anyone like to avoid all this mess all-together, but the only solution
that would be somewhat "Clean" to the code I can think of is some heavy reliance of AOP and
that is complicated to add.
   
   


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