Conversation
|
Tagging subscribers to this area: @eerhardt, @maryamariyan, @michaelgsharp Issue Detailsnull
|
|
I'm not seeing how this is a redundant check. @mapogolions, can you explain why this is redundant? |
|
@eerhardt
I also thought that if I did something wrong, then the existing quality wall should have rejected my changes. |
|
Ok, I think I get it now. It does look like checking for @adamsitnik @Tratcher - any thoughts here?
I think the other way holds - if the existing quality wall rejects your changes, you did something wrong. But it isn't guaranteed that there are tests for every scenario. We strive for it, but it isn't guaranteed. |
|
I had to stare at it for a few minutes but the change looks correct. |
| if (entry.AbsoluteExpirationRelativeToNow.HasValue) | ||
| { | ||
| if (!entry.AbsoluteExpiration.HasValue || absoluteExpiration.Value < entry.AbsoluteExpiration.Value) | ||
| var absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow.Value; |
There was a problem hiding this comment.
Since this library also ships in a .NET Standard flavor, it may be better to use Nullable<T>.GetValueOrDefault() instead of Nullable<T>.Value when Nullable<T>.HasValue returned true.
In .NET 6.0, the JIT was optimized to handle this (see @stephentoub's great blog post). The JIT in older versions of the .NET (and .NET Framework) does not have this optimization though.
There was a problem hiding this comment.
Let's do this in a follow up PR, if necessary.
eerhardt
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the contribution.
No description provided.