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
Fix: fix nullptr deletion in DeleteWindowById #8941
Conversation
afaik, calling delete on nullptr is explicitly defined to be "ok", though is definitely a bit weird. Can you elaborate on the issue this is solving? https://en.cppreference.com/w/cpp/language/delete |
I am trying to remove the w == nullptr from the condition, given that it does makes zero sense. The condition basically lets the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in the "this is pointless, leave it as-is" camp, since yes delete nullptr
is legal and defined to be a no-op. Whether this actually saves CPU cycles is difficult to answer, but any savings are unlikely to be significant.
Regardless, you need to address the compiler warnings produced at least.
Warnings fixed (but the change is still pointless)
Applied the changes to address the compiler warnings. I don't disagree with fact that delete nullptr is and will be legal. The reason this code changes still makes sense to me is because the existing code is intentionally checking if w is null to then call delete. If there was a case where a given pointer gets initialized to null and somewhere in the code we call delete for that pointer, I would be fine with that, but again in this case it's the explicit check for w being nullptr what makes it bad. |
Existing code checks intentionally for |
Hihi: just because something is "technically okay" doesn't mean it makes sense to read as developer :P What triggered me more, and what I wonder about: why is the last news window being deleted so many times? Was that a local debug, or is master doing that? That sounds a bit fishy too :D |
Mmm. So if w is null just delete, but if it's not then check the flag before delete? Still smelly to me. |
Yes absolutely. It was a dev build but I had the feeling it should happen in a release build too. Lemme take a closet look at how often and why that happens, since I saw it happening but didn't see newspaper showing up. |
In current code |
It's a simplification of |
I still see the value of changing this since as a developer I read this and it does not make much sense. |
Motivation / Problem
DeleteWindowById in window.cpp has incorrect logic since
delete w
is executed ifw == nullptr
. The delete operator is overloadad but it seems to do nothing, so I assume that got mistakenly added at some point. I've observed this debugging and when the game is idle it gets continuously called by the news loop trying to close the latest window.Description
Fixing code in DeleteWindowById that can call
delete w
where w is nullptr.Limitations
Tested by playing the game a little bit, and all works fine.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.