Skip to content
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

Codestyle: Don't use auto where not warranted #81414

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Sep 7, 2023

We allow using auto for lambdas or complex macros where a return type may change based on the parameters. But where the type is clear, we should be explicit.

https://www.youtube.com/watch?v=z87g8IQTVjw

@akien-mga akien-mga added this to the 4.2 milestone Sep 7, 2023
@akien-mga akien-mga requested a review from a team as a code owner September 7, 2023 12:41
@@ -195,7 +195,7 @@ TEST_CASE("[SceneTree][ArrayMesh] Surface metadata tests.") {
}

SUBCASE("Returns correct format for the mesh") {
auto format = RS::ARRAY_FORMAT_BLEND_SHAPE_MASK | RS::ARRAY_FORMAT_TEX_UV | RS::ARRAY_FORMAT_INDEX;
RS::ArrayFormat format = RS::ARRAY_FORMAT_BLEND_SHAPE_MASK | RS::ARRAY_FORMAT_TEX_UV | RS::ARRAY_FORMAT_INDEX;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of why auto isn't good when the type should be clear...

invalid conversion from 'int' to 'RenderingServer::ArrayFormat' [-fpermissive]

@akien-mga akien-mga force-pushed the codestyle-get-back-in-das-Auto branch from eabb977 to 7d6bb4c Compare September 7, 2023 13:38
@AThousandShips
Copy link
Member

AThousandShips commented Sep 7, 2023

What about:

const auto &location = hand_tracker->joint_locations[i];
const auto &pose = location.pose;

const auto length = map.size();

@akien-mga
Copy link
Member Author

I missed those, my regex was \tauto , I didn't think of const.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 7, 2023

Also:

for (auto &requested_extension : wrapper_request_extensions) {

That one would be KeyValue<String, bool *>

Otherwise I can't find any myself

We allow using auto for lambdas or complex macros where a return type
may change based on the parameters. But where the type is clear, we
should be explicit.

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@akien-mga akien-mga force-pushed the codestyle-get-back-in-das-Auto branch from 7d6bb4c to 1151866 Compare September 7, 2023 14:15
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@@ -2077,7 +2076,7 @@ XRPose::TrackingConfidence _transform_from_location(const T &p_location, Transfo
Basis basis;
Vector3 origin;
XRPose::TrackingConfidence confidence = XRPose::XR_TRACKING_CONFIDENCE_NONE;
const auto &pose = p_location.pose;
const XrPosef &pose = p_location.pose;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to look up what types T can take, where it's defined in openxr.h, to know which type its pose member uses. This really should be explicit for the reader.

@akien-mga akien-mga merged commit 0b9ffdf into godotengine:master Sep 7, 2023
15 checks passed
@akien-mga akien-mga deleted the codestyle-get-back-in-das-Auto branch September 7, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants