-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 unused params warnings plus incomplete struct init (done to default values). #1710
Conversation
This pull request introduces 1 alert when merging 41c3c0f into 2b64c3c - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging 0d835d2 into 2b64c3c - view on LGTM.com new alerts:
Comment posted by LGTM.com |
What is the fixme comment? |
@@ -85,7 +85,7 @@ struct lda | |||
// if it makes it into VS 2015 change the next ifdef to check Visual Studio Release | |||
|
|||
// static constexpr float underflow_threshold = 1.0e-10f; | |||
inline const float underflow_threshold() { return 1.0e-10f; } | |||
inline float underflow_threshold() { return 1.0e-10f; } |
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.
What's going on with const here?
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.
const is semantically meaningless when returning POD values as they get copied.
@@ -484,6 +484,7 @@ void enable_sources(vw& all, bool quiet, size_t passes) | |||
// background process (if foreground is not set) | |||
if (!all.opts_n_args.vm.count("foreground")) | |||
{ | |||
//FIXME switch to posix_spawn |
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.
I'm unfamiliar with posix_spawn. What's needed?
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.
Could we use a std::thread instead so that the code is portable?
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.
@JohnLangford daemon is deprecated on OSX.
Additionally posix_spawn gives fine grained control over the signal mask and all the process IDs.
It's not clear to me if those are features needed by VW particular.
@jackgerrits both daemon and posix_spawn create a new process and the former exits the parent. Threads can't provide that.
This pull request introduces 1 alert when merging dadd8b7 into 6dbb7e5 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
Merged, thanks :-) |
* Update vowpal_wabbit submodule * Pulls in these fixes - fac3c06 ("Fix crash when empty multi_ex is supplied for --cb_explore_adf (VowpalWabbit#1679)") - f085178 ("bugfix: cb_adf is not including some examples in stats calculation (VowpalWabbit#1686)") - c9d9af7 ("Fix memory leak in cb_explore_adf (VowpalWabbit#1698)") - 1e8742b ("Fix some VW initialization memory leaks (VowpalWabbit#1697)") - 75423e3 ("Fix best constant and best constant's loss calculation when using ksvm (VowpalWabbit#1704)") - 2b64c3c ("Use std::exp instead of exp free function (VowpalWabbit#1709)") - 6b7b160 ("Fix unused params warnings plus incomplete struct init (done to default values). (VowpalWabbit#1710)") - 2e1602d ("Compile fixes (VowpalWabbit#1713)") - b50fb59 ("Make cbify reduction respect is_learn parameter (VowpalWabbit#1722)") This also includes the option parsing changes - b8ab396 ("`options_i` command line parsing refactor (VowpalWabbit#1706)" - 5b020c4 ("Ataymano/mac options types fix (VowpalWabbit#1718)") There are other small or non-rl-consumable changes * Enable injection of msbuild path in Windows command-line build * Update vowpal_wabbit submodule * This pulls in the additional fix made to address the bug discovered in the previous attempt to update this. Fixed in VowpalWabbit#1742 * Updates safe_vw.cc to get rid of a no-longer-necessary cleanup step
No description provided.