Closed mojomojomojo closed 2 years ago
_nosave
variables are OK, but it would be much nicer if the state and logic could be encapsulated in a single place. Perhaps a class/struct template?The
_nosave
variables are OK, but it would be much nicer if the state and logic could be encapsulated in a single place. Perhaps a class/struct template?
There's many ways to cut this cake, but the way I would do this personally is something like
template <typename T>
class OptionValue
{
T configurationValue; /// value as stored in the Windows Registry
std::optional<T> commandLineValue; /// value specified on the command line, if any
public:
const T& get() { return commandLineValue ? *commandLineValue : configurationValue; }
// (add constructor / setters)
};
This way there is no confusion as to which value should go where, and what the order of operations (loading/saving) is. Not sure if std::optional
is available in the compiler we're using for automated builds, though.
I tried rebasing, and that's what resulted in the extra commits. Even though this seems simple, I don't seem to know what I'm doing.
OK, I rebased it for you. Run git stash save
if there are any uncommitted changes you want to keep, then run git fetch && git reset --hard mojomojomojo/config-options-from-commandline2
(replace mojomojomojo
with the name of the remote pointing to your fork) to update your local reference.
I'm really sorry I keep screwing this part up.
Is there a way to know things about the compiler environment without actually breaking the build?
I think you could look at appveyor.yml
(and the files it references) or at a build log from CI, the information will probably be there (unless AppVeyor just chooses something for us).
I don't think MSVC 2012 will support c++17.
I'll rework this change a bit. Sorry I've been treating the project like it's a bomb instead of something beautiful.
I've made some changes in the manner you've recommended.
Thank you! From a quick glance I would suggest to remove all unused methods and to use a consistent code style.
Egyptian vs. Allman braces; I see you fixed it, thanks :)
As for unused methods, it seems to me that the ones left for Overridable<T>
are the basics for utility. Even though there's nothing in there now that's using them, I used them when I was debugging the code.
Are there things I should do to get this PR better-suited for merging?
Could you please add a commit which removes unused methods? That way, if they're ever needed for debugging in the future, they can be revived by reverting the commit.
Wait, I just realized that the settings controls are initialized from the effective value, not the settings value. I'm not sure if that's right either. It avoids the UI conundrum above but replaces it with a different one - if you go into the settings, don't change anything, and exit, now future invocations of the profiler will behave differently.
This commit attempts to address the situation you've described. GUI elements in the option window show the override value and are disabled when the corresponding cmdline option is specified. The override value will not be written to the saved config.
Specifying any of these will alter the behavior of the created sleepy.exe process, but will not result in the options (saved in the registry) being changed.