Allow specifying more options from the command line

[mojomojomojo]

/minidump:<seconds_until_minidump>
/samplerate:<sample_rate>
/symsearchpath:<symbol_search_path>
/symcachedir:<symbol_cache_dir>
/usesymserver:<use_symbol_server_p>
/symserver:<symbol_server_URL>

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.

[CyberShadow]

• Please rebase, I'm seeing two almost-identical commits and a merge commit in the commit list.
• I'm seeing some mixed tabs and spaces, ideally tabs should only be used for identation and spaces for alignment.
• 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?

[CyberShadow]

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.

[mojomojomojo]

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.

[CyberShadow]

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.

[mojomojomojo]

I'm really sorry I keep screwing this part up.

[mojomojomojo]

Is there a way to know things about the compiler environment without actually breaking the build?

[CyberShadow]

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).

[mojomojomojo]

I don't think MSVC 2012 will support c++17.

[mojomojomojo]

I'll rework this change a bit. Sorry I've been treating the project like it's a bomb instead of something beautiful.

[mojomojomojo]

I've made some changes in the manner you've recommended.

[CyberShadow]

Thank you! From a quick glance I would suggest to remove all unused methods and to use a consistent code style.

[CyberShadow]

Egyptian vs. Allman braces; I see you fixed it, thanks :)

[mojomojomojo]

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.

[mojomojomojo]

Are there things I should do to get this PR better-suited for merging?

[CyberShadow]

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.

[CyberShadow]

Looks good otherwise, thanks!

[CyberShadow]

I think we need exactly one of these, not both

[mojomojomojo]

The cast operator gets used any time you try to get the value in an expression. I like this kind of thing that C++ allows, but if you prefer the verbose kind, that's what I'll do.

[CyberShadow]

Yep, the problem is when you accidentally use the wrong way to get the value, and the code doing so looks completely correct... Too much magic/sugar can be a bad thing.

[CyberShadow]

I think we need either this or SetConfigValue, not both. I would prefer the explicit method over the operator overload.

[CyberShadow]

I am on the fence if this is a good idea.

On one hand, if the profiler behaves some way, then you go into the settings, don't change anything, save & exit, and then the profiler behaves some other way, that's bad.

On the other hand, if you go into the settings and clearly see that the settings don't correspond to the profiler's current behavior, that's also bad.

Ideally the settings should indicate that some values are overridden. Maybe a pop-up message box shown when they're opened is sufficient.

Anyway, this is a pretty minor thing to worry about.

[mojomojomojo]

In order for me to observe what you're describing, I'd have to specify command-line parameters when I invoke the app, and then go into the settings. This seems like a very niche case to me.

Is there a truly correct way to handle this?

Perhaps I could disable the GUI controls in the settings if the item is specified on the command line (as well as ignore the value of the control when we update the settings when the window closes).

[CyberShadow]

That would work, but I suggest something simpler such as showing a warning if any setting is overridden, and/or just refusing to open the settings dialog at all.

[CyberShadow]

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.

[mojomojomojo]

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.

[CyberShadow]

Great, thank you.

[CyberShadow]

I misclicked at the last moment.