-
Notifications
You must be signed in to change notification settings - Fork 8
Added producer types #125
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
Added producer types #125
Conversation
bfcb453 to
56af80c
Compare
43e6cd2 to
2b75c36
Compare
4a5ed35 to
0e92483
Compare
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.
Looks like we've got all the data we need here, now to make a corresponding Producer.cpp file containing constructor implementations and with the implementations of get_profitability_yesterday and get_average_profitability_last_seven_days moved into it.
One thing to bear in mind: derived class constructors can't directly initialise their base class' variables, they have to call the base class' constructor.
Also can you rebase this onto master to include recent changes.
c53e9b8 to
773b1f4
Compare
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 would also really prefer all the maros be prefixed with OV_ or _OV_ depending on expected use case, those which you use directly should be OV_, those you usually don't should be _OV_, and something like value_or_move should be in a namespace even if its in macros like
namespace OpenVic::detail {
template<typename T>
constexpr T value_or_move(T&& value);
}
#define MACRO(Arg) ::OpenVic::detail::value_or_move(Arg);7640ad0 to
e98cd1f
Compare
Hop311
left a comment
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.
Great work! Once this is merged, "draft for factory production" #144 will automatically re-target itself onto master, however you'll still need to rebase it to fix merge conflicts.
No description provided.