Extract stable_unique to own function for clarity #1559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@JohnLangford I would like to add a test to verify stable_unique functions as expected, but it doesn't seem like there are any unit tests for the cpp code at the moment. Do you have a preferred way I can set this up? |
|
Why do we want to extract the stable_unique function into a .h file? For a function which is only used once, I tend to either leave it inline or (for clarity reasons) extract it into a separate function within the same file. |
|
Putting it in a separate file allows us to easily test it and means it can be easily consumed in future elsewhere in the lib. Considering how generic this is I think it's worth it |
lokitoth
pushed a commit
to lokitoth/vowpal_wabbit
that referenced
this pull request
Dec 20, 2019
* Extract stable_unique function * Fix unnecesarily complex type * Move stable_unique to own header * Add stable_unique test * add project to vw.sln for unit tests * Remove precompiled header from all configurations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.