rclabo commented on pull request #572: URL: https://github.com/apache/lucenenet/pull/572#issuecomment-992829479
I **strongly** prefer my variable naming of intrinsic for the additional `EventWaitHandle` over the new name of `m_signal`. The former helps throw light on the fact that it’s being used to mimic java's ability to signal on the intrinsic class which c# can’t do and hence the use of the extra`EventWaitHandle`. You changed `refreshStartGen` from a `long` to a `AtomicInt64`. I don’t believe this changes is necessary. This variable is a `long` in the original java code. You changed `isDispose` from `bool` to `AtomicBoolean`. I don’t believe this change is necessary. Reading and writing a Boolean is inherently atomic see [Is a bool read/write atomic in C#](https://stackoverflow.com/questions/59422/is-a-bool-read-write-atomic-in-c-sharp#answer-59430) You removed the holding of a lock on the `Dispose` method. This is a major drift from the Java implementation. Please note the java `close` method for this class is a synchronized method. By removing the `searchingGen = long.MaxValue; ` line in the `Dispose` method and using a conditional assignment of `searchingGen` in the `RefreshDone` method this new version of the code does not track as closely with the java code as my original ported version. This change is also why in the `Dispose` method you needed to replace the call to `Intrensic.Set()` and it’s *very* helpful comment with a call to `RefrsehDone()` which adds more drift from the java implementation. It's clear these changes improve the code but they do make it harder to compare to the java code. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
