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

Gcc warnings #1471

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@eriktorbjorn
Copy link
Member

eriktorbjorn commented Jan 1, 2019

This is not intended for immediate merge, but as a starting point for further discussion. ScummVM currently produces a lot of warnings when compiling with GCC 8. Possibly earlier versions as well. It would be good to silence the unnecessary compiler warnings, because the real warnings can easily be missed in the noise.

Most of the warnings, at the time I forked, were on the form "void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘...’; use assignment or value-initialization instead [-Wclass-memaccess]", so that's what I've focused on. The main method I've used is adding reset() methods to a number of classes and calling that instead. Is that a good approach? It seems a bit error-prone, since the method has to be updated if further members are added to the class or struct. And of course, I may have accidentally missed some as well.

There are still a couple of warnings remaining. Some of them looked non-trivial, and some were about memcpy() instead of memset(). In some cases, I didn't feel comfortable messing with that particular part of the code at the time.


void reset() {
button = 0;
pos.x = pos.y = 0;

This comment has been minimized.

@mgerhardy

mgerhardy Jan 1, 2019

Contributor

Wouldn't it be better to use the default ctor here? What if the class gets more parameters in the future. You might end up with garbage values in that case.

This comment has been minimized.

@eriktorbjorn

eriktorbjorn Jan 1, 2019

Member

I've changed the initialization of Mouse and AgiGame to what I think you meant. I did revert some such changes I had made before even committing them, but I missed those.

memset(&dirView[i], 0, sizeof(struct AgiDir));
memset(&dirSound[i], 0, sizeof(struct AgiDir));

pictures[i].flen = 0;

This comment has been minimized.

@mgerhardy

mgerhardy Jan 1, 2019

Contributor

The same (default ctor) is true here. It is imo safer to let the classes define which members must be reset.

This comment has been minimized.

@eriktorbjorn

eriktorbjorn Jan 1, 2019

Member

I'm not sure yet. AgiDir gets cleared outside of the engine constructor in some cases, and I haven't checked if that still only happens on startup. I probably won't have the time to look any further today.

@digitall

This comment has been minimized.

Copy link
Member

digitall commented Jan 1, 2019

@eriktorbjorn : Thanks for looking at this. One point, can we rebase this to squash and then split this into a commit for each engine (and backend) prior to merge please?

eriktorbjorn added some commits Jan 5, 2019

SCUMM: Silence GCC memset() warnings
Recent GCC versions complain if you memset() a class or struct that
contain non-POD data types. Get around that by either initializing
the object when created, or by adding a reset() method.
AGI: Silence GCC memset() warnings
Recent GCC versions complain if you memset() a class or struct that
contain non-POD data types. Get around that by either initializing
the object when created, or by adding a reset() method.
AGOS: Silence GCC memset() warnings
Recent GCC versions complain if you memset() a class or struct that
contain non-POD data types. Get around that by either initializing
the object when created, or by adding a reset() method.
FULLPIPE: Silence GCC memset() warnings
Recent GCC versions complain if you memset() a class or struct that
contain non-POD data types. Get around that by either initializing
the object when created, or by adding a reset() method.
BACKENDS: Silence GCC memset() warnings
Recent GCC versions complain if you memset() a class or struct that
contain non-POD data types. Get around that by either initializing
the object when created, or by adding a reset() method.

@eriktorbjorn eriktorbjorn force-pushed the eriktorbjorn:gcc-warnings branch from da4ada2 to 05f3fe4 Jan 5, 2019

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Jan 13, 2019

Looks good! +1 from me

@bluegr

bluegr approved these changes Jan 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment