Closed Idain closed 3 months ago
Yeah, I can do it. Give me a few minutes.
Thanks a lot!
I found some cases triggered by a ; TRUE
comment. I don't think we need assert TRUE == 1
, do we...?
Also, there are a lot of cases for wBattleMode
where it decreases, making the assumption WILD_BATTLE == 1
and TRAINER_BATTLE == 2
(around ~65 cases). Do I also have to add assert
s for those as well?
Have fun, @Rangi42. :)
My biggest fear is excessive assert
s cluttering the codebase.
One source of excessive asserts as exemplified by this PR is 0 and 1-index checks (and sets, i.e. xor a
).
Do we really want this? Personally, I'd argue no. Most instances of this are trivial enums that won't be reordered (e.g. WILD_/TRAINER_BATTLE), or instances where the 0 value has a special meaning (e.g. LINK_NULL).
I'd really restrict enum-related asserts to enums that are more than a handful of values, likely to warrant reordering, and skip asserts for 0 where 0 is the "nothing" value. For anything else, comments would suffice.
Years ago we considered macros like lda x
to pick between ld a, x
and xor a
depending on the value of x
. IMO this is mildly more acceptable than the assert approach, but a hard sell in terms of readability.
I'd be very against a lda
macro, where it's not clear how many bytes or cycles it would take or what the effect on flags would be. In which case I'd rather just leave out some of the more trivial assert
s.
In particular, this is a lot of assert LINK_NULL == 0
and assert WILD_BATTLE == 1
, which aren't even likely to change.
lda
would ideally only be used in places where cycle count doesn't matter, but I digress. Not a relevant topic here.
In particular, this is a lot of assert LINK_NULL == 0 and assert WILD_BATTLE == 1, which aren't even likely to change.
Exactly, I think the asserts are cluttery due to this.
I can delete those asserts without problem.
Feel free to check again. @Rangi42 @mid-kid
So, what's your verdict? @Rangi42 @vulcandth
Neat, want to do a couple dozen more?
git grep -E '(inc|dec) [abcdehl] ; [A-Z0-9_]+$'
:)Also, similar
assert
s have gone before the relevant line, which here is theinc a
. Examples: