-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Style: Apply clang-tidy
fixes
#93401
Conversation
OpenXRExtensionWrapperExtension::OpenXRExtensionWrapperExtension() : | ||
Object(), OpenXRExtensionWrapper() { | ||
OpenXRExtensionWrapperExtension::OpenXRExtensionWrapperExtension() { |
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.
This is the only bit I'm iffy on, as I'm not positive if these constructors were explicit for a reason. They should be able to setup implicitly, but there could be something about multi-inheritance classes I'm not aware of.
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.
@BastiaanOlij @m4gr3d If you wanted to take a look.
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 will probably not be a problem, I'd have to test, however on principle I don't like this change and prefer explicit notation over implicit.
The problem you introduce, especially when a large refactor is needed when someone adds new constructors to base classes, is that you introduce a ton of guess work on the compilers part and thus a ton of work on the developer.
When being explicit, it might be a little extra code, but as a developer you are communicating to a maintainer that looks at your code weeks/months/years from now, exactly what the intention was, even if the base class may have changed since.
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.
(though with the footnote that this imho would only apply when any parameters are in play, so this scenario may still be considered overkill)
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.
While I generally prefer explicit notation as well, readability-redundant-member-init
only applies when the parameters would match the default values. I can absolutely see the value in disabling it for niche cases where information about the arguments should be explicitly conveyed (ex: default values aren't necessarily zeroed-out, a constructor performs some non-trivial action, etc). For most cases though, parameters will only need to be passed if a default value is being changed.
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 think the change is good, this is the odd one out throughout the whole codebase, and it gives the impression of multi-inheritance (which is a no go for Godot) when actually OpenXRExtensionWrapperExtension
already inherits Object
, so the explicit Object
constructor is doubly redundant.
b152f33
to
e5b54ea
Compare
e5b54ea
to
0b7b52d
Compare
Overall, I agree with the changes. However, I think it's worth reassessing those related to |
• `modernize-use-default-member-init` and `readability-redundant-member-init` • Minor adjustments to `.clang-tidy` to improve syntax & remove redundancies
• `modernize-use-bool-literals`, `modernize-use-nullptr`, and `readability-braces-around-statements`
0b7b52d
to
2dcfdde
Compare
@RandomShaper While that's fair, I don't think that's something I want to touch in this PR, as it's technically a functional change. I'd prefer to tackle that in a followup whenever this gets merged |
OpenXRExtensionWrapperExtension::OpenXRExtensionWrapperExtension() : | ||
Object(), OpenXRExtensionWrapper() { | ||
OpenXRExtensionWrapperExtension::OpenXRExtensionWrapperExtension() { |
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 think the change is good, this is the odd one out throughout the whole codebase, and it gives the impression of multi-inheritance (which is a no go for Godot) when actually OpenXRExtensionWrapperExtension
already inherits Object
, so the explicit Object
constructor is doubly redundant.
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 scrolled to review the change, but the main thing is that the GitHub actions integration test is passing.
Applies a moderate amount of clang-tidy changes to the repo, many of which are only now cropping up thanks to headers recently being added as searchable locations. Parsed parimarily on Windows, with a bit of checking on Linux, so this isn't an all-encompassing pass. A notable exclusion was
cppcoreguidelines-pro-type-member-init
, as that scope is significantly larger than all other checks combined. Besides that, there were minor adjustments to.clang-tidy
to improve syntax & remove redundancies.