-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate the hash salt during object creation #237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small formatting things mostly. The tests look good, thanks for adding them.
* Used by the OLCUT configuration system, and should not be called by external code. | ||
*/ | ||
@Override | ||
public synchronized void postConfig() throws PropertyException{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These postConfigs don't access any other resources under synchronization elsewhere in the class, so we don't need to make them synchronized to avoid static analysis false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it makes sense to only apply synchronization as needed to safely modify an attribute. I think I may have misunderstood that from a previous discussion. Thanks for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the false positive only arises in Trainer
s as those also access the RNG field under synchronization. In practice as postConfig
is always only called either from the constructor, or from OLCUT before the object is published it doesn't actually need synchronization, but the static analysis tools just see a public method and so warn on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
* Validate the hash salt during object creation * Changes to address code review feedback
Description
These changes add validations of the hash salt during object creation, either programmatically or via the Configuration System.
Motivation
These changes fix #233