BUILD: Reduce GCC warnings #1471
Conversation
engines/agi/agi.h
Outdated
|
|
||
| void reset() { | ||
| button = 0; | ||
| pos.x = pos.y = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
The same (default ctor) is true here. It is imo safer to let the classes define which members must be reset.
There was a problem hiding this comment.
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.
|
@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? |
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.
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.
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.
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.
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
force-pushed
the
gcc-warnings
branch
from
da4ada2
to
05f3fe4
Compare
|
Looks good! +1 from me |
|
Any news on this? Are there any further actions to be taken? If not, we can merge this |
|
The commits with fixes to BACKENDS, FULLPIPE and AGOS look correct and reasonably trivial... The SCUMM and AGI engine changes are slightly more involved and probably should be reviewed in detail, including the AGI change maybe needing further changes as per @eriktorbjorn 's comment. |
|
I'm an outsider here, but it looks to me like memset is less error-prone than a reset function (you will never miss fields to reset), plus it's probably faster (unless the compiler is really smart). |
|
It all makes sense to me. Even with the reset() method. It is safer like this than the memset(), and we have Coverity for spotting uninitialized field. |
|
Any objections against merging this? |
|
The commits for SCUMM, AGOS, FULLPIPE and BACKENDS were quite straightforward, so I went ahead and merged them separately. The commit for AGI would need more work concerning the AgiDir structures |
STARK: Issue fixing
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.