Attention is currently required from: Raul Rangel, Reka Norman, Nick Vaccaro, Karthik Ramasubramanian. Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62905 )
Change subject: util/spd_tools: Add support for exclusive IDs ......................................................................
Patch Set 5:
(6 comments)
File util/spd_tools/src/part_id_gen/part_id_gen.go:
https://review.coreboot.org/c/coreboot/+/62905/comment/c62812d4_17d905c3 PS3, Line 96: mappingType
Do we need a new type
Strictly speaking no, but is safer since mappingType will be represented as an int but attempting to do arithmetic or index an array with values of that type will result in a compiler error.
https://review.coreboot.org/c/coreboot/+/62905/comment/91643bbb_4f0ab664 PS3, Line 99: mappingType
Not required.
Done
https://review.coreboot.org/c/coreboot/+/62905/comment/c32ae563_77fb031e PS3, Line 102:
This can be: […]
Done
https://review.coreboot.org/c/coreboot/+/62905/comment/e6cf4077_658ba0da PS3, Line 191: fields[1]) >= 2
IS this check required?
If you have a row formatted like `part-id,` then `field[1]` will existing but will be an empty string.
https://review.coreboot.org/c/coreboot/+/62905/comment/836684e4_e1e3a0f1 PS3, Line 284: map[int]mappingType{}
Feels weird to use a map for the concerned use-case. Feels like an array is sufficient. […]
Done
https://review.coreboot.org/c/coreboot/+/62905/comment/d7eb1cbe_2cf1a3e7 PS3, Line 314: if mapping, present := assignedMapping[p.index]; present { : // Don't allow explicit and non-explicit parts to use the same index : if mapping != p.mapping && (mapping == Exclusive || p.mapping == Exclusive) { : return nil, fmt.Errorf("Exclusive/non-exclusive conflict in assigning %s to ID %d", p.partName, p.index) : } : } else { : assignedMapping[p.index] = p.mapping : }
if planning to switch assignedMapping as array, this can be changed as: […]
Done
-
Robert Zieba (Code Review)