Add logger, improve support for ARM64 & Windows 7 #13
Conversation
modmuss50
changed the title
|
|
||
| class Bootstrap { | ||
| public: | ||
| explicit Bootstrap(const std::shared_ptr<ISystemHelper>& systemHelper); | ||
| explicit Bootstrap(ISystemHelper& systemHelper, Logger& logger); |
There was a problem hiding this comment.
| explicit Bootstrap(ISystemHelper& systemHelper, Logger& logger); | |
| Bootstrap(const ISystemHelper& systemHelper, Logger& logger); |
| private: | ||
| std::shared_ptr<ISystemHelper> systemHelper; | ||
| ISystemHelper& systemHelper; |
There was a problem hiding this comment.
| ISystemHelper& systemHelper; | |
| const ISystemHelper& systemHelper; |
There was a problem hiding this comment.
Thanks for taking a look at this :) I have fixed the other points.
I was told a while back that you generally shouldn't have const refrences as member vars, ill be honest I didn't fully understand the reasoning. There seems to be a bit of litrature about this online such as: https://lesleylai.info/en/const-and-reference-member-variables/ And then other places seem to suggest having const is fine.
The orignal design around this is to allow a mock of ISystemHelper for unit tests (that dont exist atm), the formal name for this pattern seems to be "dependency injection via constructor".
Going back to using a shared_ptr seems like it might solve most of the possible problems? Either way I'm not too worried about being 100% correct as long as it works, but it is an intresting point of conversation.
There was a problem hiding this comment.
It's generally bad practice to have (even non-const) references as member variables, as that article explains, because references cannot be rebound, i.e. are effectively const. There is no need to change back to shared_ptr, if you want you can use std::reference_wrapper instead, but I left this out of the review because I think the codebase is too simple for this to become a problem.
| @@ -44,92 +39,147 @@ void Bootstrap::launch() { | |||
| } | |||
|
|
|||
| bool Bootstrap::launchMinecraftLauncher() { | |||
| if (auto minecraftLauncherPath = systemHelper->getRegValue(HKEY_CURRENT_USER, MC_LAUNCH_REG_PATH, MC_LAUNCH_REG_KEY); minecraftLauncherPath) { | |||
| const Architecture::Value hostArch = systemHelper.getHostArchitecture(); | |||
| logger.log(L"Host archiecture: " + Architecture::AsString(hostArch)); | |||
There was a problem hiding this comment.
Opps, I will open an issue to remind myself to fix this in the next version. Not a major worry as it only shows in the log file 👍
KB2533623update.