Skip to content
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

Fix math::RandomSeed() BINDING_TYPE macro condition #1462

Merged
merged 4 commits into from Jul 9, 2018

Conversation

Projects
None yet
3 participants
@manish7294
Copy link
Member

manish7294 commented Jul 4, 2018

Check whether the BINDING_TYPE is NULL or not. The order of mlpack_main.cpp inclusion is also corrected all over the codebase.

manish7294 added some commits Jul 4, 2018

@@ -39,7 +39,7 @@ extern MLPACK_EXPORT std::normal_distribution<> randNormalDist;
*/
inline void RandomSeed(const size_t seed)
{
#if (BINDING_TYPE != BINDING_TYPE_TEST)
#if BINDING_TYPE == BINDING_TYPE_UNKNOWN || BINDING_TYPE != BINDING_TYPE_TEST

This comment has been minimized.

@rcurtin

rcurtin Jul 4, 2018

Member

I think it's probably better to use defined(BINDING_TYPE) instead of BINDING_TYPE == BINDING_TYPE_UNKNOWN.

@rcurtin

rcurtin approved these changes Jul 5, 2018

Copy link
Member

rcurtin left a comment

Ah, I see what you mean about the include ordering issue, so we should definitely incorporate that. Still, the !defined(BINDING_TYPE) will help for other cases where for some reason the macro is not defined (for instance, if a user writes code that doesn't include things in the right order).

I'll go ahead and merge this in 3 days to leave time for any other comments. Thanks!

@manish7294

This comment has been minimized.

Copy link
Member Author

manish7294 commented Jul 5, 2018

Right, that's exactly why I kept !defined(BINDING_TYPE) there :)

@zoq

zoq approved these changes Jul 6, 2018

Copy link
Member

zoq left a comment

Thanks for looking into the issue, no comments from my side.

@rcurtin rcurtin merged commit f75026f into mlpack:master Jul 9, 2018

5 checks passed

Memory Checks
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Jul 9, 2018

Thanks again 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.