Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add logger, improve support for ARM64 & Windows 7 #13
Add logger, improve support for ARM64 & Windows 7 #13
Changes from 3 commits
83ae0e7
648adb9
602732b
47fd38c
f4a8b12
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usestd::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.