Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/11/it85spi.c@77 PS11, Line 77: unsigned int shm_io_base;
In existing code, wrapping this with ifdef-endif was needed, because otherwise compiler would produc […]
Similar to the `const` case, the compiler is doing its job correctly, so let's think about what it is trying to tell us:
If we remove the ifdef-endif block around the `shm_io_base` variable declaration in the original code, and build with `LPC_IO` not defined, then the compiler will rightly warn us that shm_io_base was "unused" (ie we've reserved some static memory in .bss that are never accessed, thus we could save some memory by not declaring this static global variable at all).
In other words, the compiler is telling us that we needn't define this variable at all.
In the new code, `shm_io_base` has now become a struct field. Thus, the compiler won't reserve any memory for it in `.bss` at build time. Instead, the effect is now that any structs of this type that are created at runtime will now have this little extra bit of memory allocated for this field - but the field will still be unused! The compiler, however, isn't able to detect this, and thus doesn't throw a warning ... but this is still unused memory.
It isn't a huge deal in this little instance, more of a lesson in how to do a refactoring patch like this: 1) try to keep the code consistent with the original code 2) try to avoid allocating memory that won't be used
Now, you could argue that using "ifdef/endif" like this is brittle, overly complex, and the memory savings are tiny and an over-optimization that has no practical value - and I would generally agree. But, let's clean that up in a separate patch that globally replaces this compile-time logic with equivalent run-time logic that doesn't rely on the C pre-processor.