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

Initialize class/struct variables with default values in core/ and drivers/ #43730

Merged
merged 1 commit into from
Nov 24, 2020

Conversation

qarmin
Copy link
Contributor

@qarmin qarmin commented Nov 20, 2020

Part of #43636

If this commit break anything, it would mean that the previous behavior was based on the undefined behavior of C++(concerning uninitialized memory) but from my tests there is no difference in engine behavior.

Numbers of Cppcheck errors decreased from 812 to 743

@qarmin qarmin added this to the 4.0 milestone Nov 20, 2020
@qarmin qarmin requested a review from hpvb as a code owner November 20, 2020 20:33
@qarmin qarmin force-pushed the core_drivers_default_values branch 4 times, most recently from ceafcef to 8d3f494 Compare November 20, 2020 20:51
core/math/expression.h Outdated Show resolved Hide resolved
core/math/expression.h Outdated Show resolved Hide resolved
core/math/quick_hull.h Outdated Show resolved Hide resolved
core/os/dir_access.h Outdated Show resolved Hide resolved
core/os/file_access.cpp Outdated Show resolved Hide resolved
int width = 0;
int height = 0;
uint32_t flags = 0;
Image::Format format = Image::Format::FORMAT_MAX;
Copy link
Member

Choose a reason for hiding this comment

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

Compilers other than MSVC used to yell as using scoped enums like this. But maybe this is now supported by C++11 or later?

@qarmin qarmin force-pushed the core_drivers_default_values branch from 8d3f494 to 8dab9fb Compare November 23, 2020 11:54
core/io/udp_server.h Outdated Show resolved Hide resolved
@qarmin qarmin force-pushed the core_drivers_default_values branch from 8dab9fb to 47e84c0 Compare November 23, 2020 15:54
@@ -252,10 +252,10 @@ String FileAccess::get_token() const {

class CharBuffer {
Vector<char> vector;
char stack_buffer[256];
char stack_buffer[256]; // It does not need to be initialized with zeroes, because it is always used in the correct way.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a special comment you can use to disable the cppcheck warning, so that it's still resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not suppress Cppcheck's warning, but it does inform why this board is not initiated with zeros.
I wrote it thinking it's a single case, but at the moment I found already ~10 such large arrays, so it makes no sense to describe the same thing with each one, so I also removed this comment.

Copy link
Member

Choose a reason for hiding this comment

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

Yes we discussed it briefly on IRC and the general consensus is that buffers should always stay uninitialized: https://freenode.logbot.info/godotengine-devel/20201123#c5936108-c5937054

I think it could be useful to have a standard comment we can put in such case to make things explicit.

And most linters have specific comments that can be used to turn off warnings on a specific line, maybe cppcheck has that too? If so we could just use that.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks that suppression of uninitMemberVar doesn't work.
I tried to put it comment where variable is created and also in constructor, but none of this worked.

Copy link
Member

Choose a reason for hiding this comment

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

Did you use --inline-suppr on the command line as suggested in that thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use cppcheck with and without this command, but there wasn't any difference

Copy link
Member

Choose a reason for hiding this comment

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

I tested and it seems to work, but the suppress comment needs to be on the line which triggers it, which is sadly not the variable declaration, but the constructor. This works to suppress two warnings:

class Test {
public:
	int a;

	Test() {} // cppcheck-suppress uninitMemberVar
};

int main() {
	Test t = Test(); // cppcheck-suppress unreadVariable
	return 0;
}
$ cppcheck --enable=all test.cpp 
Checking test.cpp ...
test.cpp:5:2: warning: Member variable 'Test::a' is not initialized in the constructor. [uninitMemberVar]
 Test() {} // cppcheck-suppress uninitMemberVar
 ^
test.cpp:9:9: style: Variable 't' is assigned a value that is never used. [unreadVariable]
 Test t = Test(); // cppcheck-suppress unreadVariable
        ^
$ cppcheck --enable=all --inline-suppr test.cpp 
Checking test.cpp ...

Copy link
Member

@akien-mga akien-mga Nov 24, 2020

Choose a reason for hiding this comment

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

But since it would silence the error for all member variables, I'm not sure we should use it.

Maybe there's another way to list false positives / intentionally ignored warnings without using the suppress comment?

@akien-mga akien-mga merged commit 32b31a5 into godotengine:master Nov 24, 2020
@akien-mga
Copy link
Member

Thanks!

@qarmin qarmin deleted the core_drivers_default_values branch January 8, 2021 07: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.

4 participants