Add assertions throughout the code to provide guarantees (not just comments)

[Rangi42]

Some code is written assuming that a constant has a certain value; or is divisible by some quantity; or that one struct field is a certain offset from another; or that two maps are in the same group; or two graphics in the same bank; and so on. These can be documented with assert better than with comments.

[mid-kid]

The main issue I have with this is xor a for ld a, 0 and and a for cp 0, where a certain constant is assumed to be 0. This is currently commented and I think asserts would hurt readability of functions where these are used a lot.
As for divisibility, I'm pretty sure you're talking about alignment, and that can be better enforced by the linker using the ALIGN parameter like we're already doing. for const/enums we always just set the value for the next item, or change the increment, which I think is appropriate enough.
Similarly, graphics being in the same bank should be enforced in the same way that two functions being in the same bank is: Putting them in the same section. If this is not possible, do what #631 does and make a constant and hardcode the BANK[] of their respective sections, as a stopgap for gbdev/rgbds#244.

I believe assert should be a last-resort solution for things that can't be expressed in any "natural" way: Some function expecting two maps to be in the same group instead of checking their groups separately being the prime example.
Just be careful and avoid using asserts for things like hli where CONST+1 must equal CONST2 (if you change a structure you should look for all uses of it. If anything, the disassembly should make these sorts of structures more explicit instead of just loose wram labels), or to check an array is the right size. It's assumed that if you modify a structure or an array length, things will break, and comments are enough for easy grepability and readability.

[Kroc]

If asserts can be placed in macros then to some extent, the asserts can be automatic in places where the assembly code makes assumptions about order / value. Trust me, you do not want to be debugging an issue where an order or value assumption changes, it can be completely invisible in the code.

[aaaaaa123456789]

I think obvious "none" constants don't need any assert. It should be obvious that MUSIC_NONE equals 0, and anyone who changes something that basic should be very aware of the effects anyway.

But there are many cases where a constant is 0 because it's simply the first field of a struct or the first element in an enum, and those might be worth asserting.

[Rangi42]

I agree that zero constants are generally obvious, like NONEs, so xor a and cp a are fine with just comments (to at least make the constant greppable for that line). Likewise, if something can be made explicit by subtracting two struct values, or aligning a section, or actually making use of the constants we've defined, then it should.

(For example, ld bc, 5 + 4 * 2 ; Location of the level of the 5th wild Pokemon in that map in wildmons.asm -- it's not at all clear that the 5 is meant to skip the map ID and morn/day/nite encounter rates, and that the 4 is really 5 − 1, with 2 being the width of a "db level, species" entry. Or )

Anyway, asserts are for last-resort cases. Some examples of where I think they'd be more helpful than comments:

; MOUNT_MOON_SQUARE and TIN_TOWER_ROOF are outdoor maps within indoor maps.
; Dig and Escape Rope should not take you to them.
	ld a, [wPrevMapGroup]
	cp GROUP_MOUNT_MOON_SQUARE ; aka GROUP_TIN_TOWER_ROOF
	jr nz, .not_mt_moon_or_tin_tower
	ld a, [wPrevMapNumber]
	cp MAP_MOUNT_MOON_SQUARE
	ret z
	cp MAP_TIN_TOWER_ROOF
	ret z
.not_mt_moon_or_tin_tower

Suggestion: assert GROUP_MOUNT_MOON_SQUARE == GROUP_TIN_TOWER_ROOF

	ld hl, OBJECT_FACING_STEP
	add hl, bc
	and 2
	ld a, FACING_BOULDER_DUST_1
	jr z, .ok
	inc a ; FACING_BOULDER_DUST_2
.ok
	ld [hl], a
	ret

Suggestion: assert FACING_BOULDER_DUST_1 + 1 == FACING_BOULDER_DUST_2

	; Instead of deleting the sprites, make them all use PAL_BATTLE_OB_ENEMY
	ld hl, wVirtualOAMSprite00Attributes
	ld c, NUM_SPRITE_OAM_STRUCTS
.loop
	ld a, [hl]
	and $ff ^ (PALETTE_MASK | VRAM_BANK_1) ; PAL_BATTLE_OB_ENEMY (0)
	ld [hli], a
rept SPRITEOAMSTRUCT_LENGTH - 1
	inc hl
endr
	dec c
	jr nz, .loop
	ret

Suggestion: assert PAL_BATTLE_OB_ENEMY == 0 (it's not obvious that this would be zero, nor that this mask is implicitly zeroing out the palette bits).

	ld hl, ChrisCardPic
	ld a, [wPlayerGender]
	bit PLAYERGENDER_FEMALE_F, a
	jr z, .GotClass
	ld hl, KrisCardPic
.GotClass:
	ld de, vTiles2 tile $00
	ld bc, $23 tiles
	ld a, BANK(ChrisCardPic) ; aka BANK(KrisCardPic)
	call FarCopyBytes

Suggestion: assert BANK(ChrisCardPic) == BANK(KrisCardPic) (Some of these aka-BANK comments can and should be using BANK("The section both things are in"), perhaps with more finely-divided sections so a few related graphics all get their own section; but sometimes an assert` might be simpler than splitting sections even further.)

BuenasPasswordTable:
; there are NUM_PASSWORD_CATEGORIES entries
	dw .JohtoStarters
	dw .Beverages
	dw .HealingItems
	dw .Balls
	dw .Pokemon1
	dw .Pokemon2
	dw .JohtoTowns
	dw .Types
	dw .Moves
	dw .XItems
	dw .RadioStations

Suggestion: assert (BuenasPasswordTable.End - BuenasPasswordTable) / 2 == NUM_PASSWORD_CATEGORIES (or better yet, NUM_PASSWORD_CATEGORIES EQUS "((BuenasPasswordTable.End - BuenasPasswordTable) / 2)"; but IIRC I tried that when first defining NUM_PASSWORD_CATEGORIES and it was not resolvable at build time, at least not by then-current rgbds.)

[Rangi42]

Assertions are also ideal for code that was optimized by assuming certain conditions. pokecrystal itself never uses any sbc a tricks, but if it did:

	; a = carry ? FOO : BAR
	sbc a
	assert FOO + 1 == BAR
	add BAR

[Rangi42]

Another: zero constants are generally obvious, one constants less so.

LoadContestantName:
; If a = 1, get your name.
	dec a ; BUG_CONTEST_PLAYER
	jr z, .player

Suggestion: assert BUG_CONTEST_PLAYER == 1

[Rangi42]

Assertions can also be used for sanity checks of macro arguments, and to give better error messages.

channel_count: MACRO
assert \1 >= 1 && \1 <= 4, "channel_count must be 1-4"
_num_channels = \1 - 1
ENDM

rest: MACRO
assert \1 >= 1 && \1 <= 16, "rest must be 1-16"
	note 0, \1 ; length
ENDM

map_id: MACRO
;\1: map id
assert DEF(GROUP_\1) && DEF(MAP_\1), "Undefined map_const \1 in constants/map_constants.asm"
	db GROUP_\1, MAP_\1
ENDM

[mid-kid]

Macros should definitely be checked for value ranges, but that's a pain that nobody's bothered to do yet.
I agree with most of these suggestions except for one:
BuenasPasswordTable seems to be something that should have a counterpart const array, like mons, items and moves, do, instead of calculating it. It would be nice to have a way to define both things in the same place but alas, we don't. We rely on such tables having the same size across all of the repo.

[Rangi42]

Good idea for BuenasPasswordTable. There are other structs for which we haven't defined constant offsets yet (such as warp_events or bg_events; home/map.asm uses some offsets for those). Those will turn up during code cleanup when looking for remaining magic numbers.

Speaking of offsets, there might be some others which are currently defined as plain const sequences, but which could be taking advantage of a corresponding WRAM struct to stay in sync. Like how base data works: BASE_GENDER EQUS "(wBaseGender - wCurBaseData)" instead of const BASE_GENDER ; $0d.

[Rangi42]

Another bank assertion: PokemonPicPointers == UnownPicPointers

[Rangi42]

The radio random pointer tables that have size 16 and thus don't use a rejection sampling loop could also use assertions.

[Rangi42]

Most of the assertions discussed here have been added in PR #776. There are probably opportunities for more, and the ones added should be reviewed.

[Rangi42]

One place where assertions would be helpful is for wild data. Each map requires exactly the right amount of entries, and too many/few will corrupt that map and all subsequent ones. Consider refactoring this:

	map_id SPROUT_TOWER_2F
	db 2 percent, 2 percent, 2 percent ; encounter rates: morn/day/nite
	; morn
	db 3, RATTATA
	...
	; day
	db 3, RATTATA
	...
	; nite
	db 3, GASTLY
	...

into this:

	def_wilds SPROUT_TOWER_2F
	db 2 percent, 2 percent, 2 percent ; encounter rates: morn/day/nite
	; morn
	db 3, RATTATA
	...
	; day
	db 3, RATTATA
	...
	; nite
	db 3, GASTLY
	...
	end_wilds

end_wilds would check the distance to the previous def_wilds and assert that it's exactly GRASS_WILDDATA_LENGTH bytes.

[Rangi42]

Other assertions could be added at the end of various data tables. For example, in data/maps/roofs.asm, MapGroupRoofs is supposed to have one entry per map group (plus an extra 0th one on top), but it's easy to forget to update when adding a new group. Consider:

MapGroupRoofs:
; entries correspond to map groups
; values are indexes for Roofs (see below)
	db -1             ;  0
	db ROOF_OLIVINE   ;  1 (Olivine)
	...
	db ROOF_NEW_BARK  ; 26 (Cherrygrove)

	assert @ - MapGroupRoofs == NUM_MAP_GROUPS

This could be generalized to start_table and end_table macros.

[Rangi42]

Regarding general-purpose table macros:

table_width: MACRO
CURRENT_TABLE_WIDTH = \1
CURRENT_TABLE_START = @
ENDM

check_table_length: MACRO
_length = \1
_expected = CURRENT_TABLE_WIDTH * _length
_actual = @ - CURRENT_TABLE_START
if _expected != _actual
    fail "Expected {d:_length} entries totaling {d:_expected} bytes, got {d:_actual} bytes"
endc
ENDM