Add more assert cases in the codebase

[Idain]

No description provided.

[Rangi42]

Neat, want to do a couple dozen more? git grep -E '(inc|dec) [abcdehl] ; [A-Z0-9_]+$' :)

Also, similar asserts have gone before the relevant line, which here is the inc a. Examples:

	assert PokemonPicPointers == UnownPicPointers
	ld hl, PokemonPicPointers

	assert BUG_CONTEST_PLAYER == 1
	dec a

	assert LOW(SERIAL_LINK_BYTE_TIMEOUT) == 0
	xor a ; LOW(SERIAL_LINK_BYTE_TIMEOUT)

[Idain]

Yeah, I can do it. Give me a few minutes.

[Rangi42]

Thanks a lot!

[Idain]

I found some cases triggered by a ; TRUE comment. I don't think we need assert TRUE == 1, do we...?

[Idain]

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 asserts for those as well?

[Idain]

Have fun, @Rangi42. :)

[mid-kid]

My biggest fear is excessive asserts 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.

[mid-kid]

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.

[Rangi42]

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 asserts.

In particular, this is a lot of assert LINK_NULL == 0 and assert WILD_BATTLE == 1, which aren't even likely to change.

[mid-kid]

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.

[Idain]

I can delete those asserts without problem.

[Idain]

Feel free to check again. @Rangi42 @mid-kid

[vulcandth]

I am all for asserts, but not for every little thing. I think most of these that are left are fine. Basically we want to leave it to instances where people are likely to change, and likely to miss something when doing so.

[Idain]

So, what's your verdict? @Rangi42 @vulcandth

[vulcandth]

I looked it over.. Looks fine to me. I'll give a chance for other's to add final comments and then i'll merge if nobody objects.

[Rangi42]

Thank you!