search

icculus / Serious-Engine

An open source version of a game engine developed by Croteam for the classic Serious Sam games.
GNU General Public License v2.0
165 stars 23 forks source link

Fix some warnings #45

Closed emlai closed 8 years ago

DanielGibson commented 8 years ago

oh.. looks like we might have done some duplicate work: https://github.com/rcgordon/Serious-Engine/pull/44

emlai commented 8 years ago

Wow, how is it possible that we opened the PRs only 2 seconds apart 😮

Anyway, I saw you fixed -Wreorder while I was working on my branch so I didn't touch that, but there's still some overlap. And we fixed some things differently. But overall I think the overlap seems to be relatively small.

icculus commented 8 years ago

There were two competing PRs for this before; @DanielGibson, did we decide this PR should go in, too? Let me know and I'll accept it.

DanielGibson commented 8 years ago

I'd suggest merging my PR first and then cherry-picking the parts of this which aren't duplicates of my fixes - thankfully it seems like @emlai split up the fixes into meaningful commits, like a professional :-)

(I can do the cherry picking etc, I could create a new PR with the remaining commits of this one then)

DanielGibson commented 8 years ago

I also wonder why all those if(this != NULL) checks were there if that doesn't really happen anyway?

DanielGibson commented 8 years ago

I can do the cherry picking etc, I could create a new PR with the remaining commits of this one then

seeing my stuff is merged now, unless someone beats me to it I plan to do the merging/cherry-picking this weekend

DanielGibson commented 8 years ago

I created a new pull request with all the still relevant fixes from this one, see #54

icculus commented 8 years ago

Merged #54, closing this one. Thanks, everyone!

DanielGibson commented 8 years ago

wow, that was fast, thanks a lot :)