Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41440 )
Change subject: sconfig: Add support for firmware configuration ......................................................................
Patch Set 1:
(7 comments)
Thanks, I think this approach is a lot better!
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@336 PS1, Line 336: strncasecmp I don't think you want to make these case insensitive. The macro names are gonna be case sensitive, so if people start mixing up the case things are probably going to break at one point anyway. Better to catch it early.
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@372 PS1, Line 372: nit: maybe sanity-check bits (start <= end, end < 32)?
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@373 PS1, Line 373: if (!field) { Shouldn't this be a fatal error if field already exists (not just silently ignore)?
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@392 PS1, Line 392: option = option->next; nit: might also want to check that option name doesn't exist yet while doing this
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@394 PS1, Line 394: } nit: should probably sanity check add->value < 1 << field->end_bit
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/sconfig.y File util/sconfig/sconfig.y:
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/sconfig.y@79 PS1, Line 79: NUMBER nit: could consider making the second number optional for single-bit fields?
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/sconfig.y@85 PS1, Line 85: strtoul($<string>3, nit: off-topic, but I'm not sure why we're using strto(u)l for all the numbers in here. According to my (limited) understanding of yacc, it should be possible to just say $<number>3 here (and that would fail visibly for wrong tokens, whereas strtoul() just returns 0 on parsing errors).