pre/post: reworked changes in AddToFullPack for correctness and portability #551
+97
−60
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cific flag, not the old states of all flags I know this might only be partially suitable for this pull request (due to the addition of the SET_OR_UNSET_FLAG macro), but I'm really too lazy to split it into a new pull request and then rebase it for this little thing.
SmileyAG
commented
Aug 12, 2024
| ent->v.iuser1 = oldIUser1; | ||
| ent->v.iuser2 = oldIUser2; | ||
| if (status != BXT_ADDTOFULLPACK_STATE_NO) { | ||
| ent->v.effects = SET_OR_UNSET_FLAG(oldEffectsNodraw, ent->v.effects, EF_NODRAW); |
There was a problem hiding this comment.
Damn, how did I let this happen and the compiler didn't even complain? Will fix it later simply to:
SET_OR_UNSET_FLAG(oldEffectsNodraw, ent->v.effects, EF_NODRAW);
There was a problem hiding this comment.
Not sure I would've caught this. It's difficult to review when two different changes are bundled together in one commit like this, especially when the diff is completely messed up too
YaLTeR
reviewed
Sep 16, 2024
| ent->v.iuser1 = oldIUser1; | ||
| ent->v.iuser2 = oldIUser2; | ||
| if (status != BXT_ADDTOFULLPACK_STATE_NO) { | ||
| ent->v.effects = SET_OR_UNSET_FLAG(oldEffectsNodraw, ent->v.effects, EF_NODRAW); |
There was a problem hiding this comment.
Not sure I would've caught this. It's difficult to review when two different changes are bundled together in one commit like this, especially when the diff is completely messed up too
| #define GET_PEV(thisptr) *reinterpret_cast<entvars_t**>(reinterpret_cast<uintptr_t>(thisptr) + off_pev); | ||
| #define SET_OR_UNSET_FLAG(boolVar, setVar, flag) boolVar ? (setVar |= flag) : (setVar &= ~flag); |
There was a problem hiding this comment.
Suggested change
| #define SET_OR_UNSET_FLAG(boolVar, setVar, flag) boolVar ? (setVar |= flag) : (setVar &= ~flag); | |
| #define SET_OR_UNSET_FLAG(boolVar, setVar, flag) (boolVar) ? (setVar |= (flag)) : (setVar &= ~(flag)); |
Otherwise it's possible to get unintended splitting.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Three reasons why this request exists:
SV_AddToFullPackSo using such a pre/post implementation, we can easily make the same changes in several similar functions, i.e. no need to stupidly copy-paste the code every time
We return old states only when they have actually been changed, thereby removing the possibility of accidentally overwriting the new state for objects to which we should not even make changes!
For integer variables that serve to store flags for performing bitwise operations with them, we now return not their entire old state, but only the old state of a specific flag that we changed!