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

[3.x] Initialize class variables with default values #44125

Closed
wants to merge 1 commit into from

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Dec 5, 2020

Part of #43636 for 3.x branch.

It should decrease number of Cppcheck errors from 791 to ~125(except buffers and some rendering things).

It should be really easy to check this commit, because it adds only to constructor values initializations(in opposite to similar commits to master branch when I moved a lot of things across code and also initialized structs members which was not shown in Cppcheck).

@qarmin qarmin added this to the 3.2 milestone Dec 5, 2020
@qarmin qarmin force-pushed the initialize_variables branch 2 times, most recently from 6392ef8 to 02463ad Compare December 5, 2020 18:48
@qarmin qarmin force-pushed the initialize_variables branch 2 times, most recently from 8681e30 to 6e24d66 Compare January 22, 2021 20:54
@qarmin qarmin force-pushed the initialize_variables branch from 6e24d66 to d2b40ca Compare January 30, 2021 12:24
@@ -43,6 +43,12 @@
Generic6DOFJointBullet::Generic6DOFJointBullet(RigidBodyBullet *rbA, RigidBodyBullet *rbB, const Transform &frameInA, const Transform &frameInB) :
JointBullet() {

for (int i = 0; i < 3; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been fixed with a different code which is a bit cleaner on master:

for (int i = 0; i < 3; i++) {
	for (int j = 0; j < PhysicsServer3D::G6DOF_JOINT_FLAG_MAX; j++) {
		flags[i][j] = false;
	}
}

Also, I'm wondering if memset would be better in this case. I don't know if all compilers would optimize this loop properly.

@akien-mga
Copy link
Member

That's a huge PR with a lot of surface for potential bugs. I'll try to review it soon but it takes time.

To fix #35877, it would be better to make a dedicated PR with only that fix.

@qarmin qarmin force-pushed the initialize_variables branch from 50fa21e to 628b620 Compare February 9, 2021 14:30
@qarmin qarmin requested review from a team as code owners March 12, 2021 12:26
@qarmin qarmin requested review from a team as code owners March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@akien-mga akien-mga modified the milestones: 3.2, 3.3 Mar 17, 2021
@akien-mga akien-mga changed the title [3.2] Initialize class variables with default values [3.x] Initialize class variables with default values Mar 26, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@qarmin qarmin marked this pull request as draft August 15, 2021 20:29
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I assume this is still desired, but it needs to be rebased on the latest 3.x branch. Also note that #35877 has been fixed via #45803.

@@ -980,6 +980,8 @@ ResourceInteractiveLoaderBinary::ResourceInteractiveLoaderBinary() :
f(NULL),
error(OK),
stage(0) {
ver_format = 0;
importmd_ofs = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why are some of these before the { and some after it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initializer list(before {) require to have all variables in same order as in class definition, so for me it was easier to write such code(in master initialization was moved from constructor)

@Chaosus Chaosus modified the milestones: 3.4, 3.5 Nov 8, 2021
@qarmin
Copy link
Contributor Author

qarmin commented Nov 30, 2021

Currently I don't have enough time to rebase such commits, so feel free to use them to create new PR.

@qarmin qarmin closed this Nov 30, 2021
@qarmin qarmin removed this from the 3.5 milestone Nov 30, 2021
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.

5 participants