Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62898 )
Change subject: hwaccess: replace macros by C code ......................................................................
Patch Set 3:
(5 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62898/comment/e44886c7_43abb136 PS2, Line 9: a `one file` to match the later `one [file]`?
https://review.coreboot.org/c/flashrom/+/62898/comment/7b4327f4_5963f9f9 PS2, Line 11: endian endianness
Patchset:
PS2:
My goal is to compile a file for little endian or one for big endian without the need to provide a s […]
The C compiler can only inline what it can see, i.e. what is in header files. To optimize across compilation units, we'd need LTO.
File hwaccess.h:
https://review.coreboot.org/c/flashrom/+/62898/comment/7010849f_5309eea6 PS2, Line 84: #define be_to_cpu8 cpu_to_be8 : #define be_to_cpu16 cpu_to_be16 : #define be_to_cpu32 cpu_to_be32 : #define be_to_cpu64 cpu_to_be64 : #define le_to_cpu8 cpu_to_le8 : #define le_to_cpu16 cpu_to_le16 : #define le_to_cpu32 cpu_to_le32 : #define le_to_cpu64 cpu_to_le64 : IMO, this part seems easy to understand and spares us error-prone repetitions in the .c files.
https://review.coreboot.org/c/flashrom/+/62898/comment/ea8c3f30_f81bc571 PS2, Line 28: uint8_t swap8 (const uint8_t value); : uint16_t swap16(const uint16_t value); : uint32_t swap32(const uint32_t value); : uint64_t swap64(const uint64_t value); How about making these `static inline`? I'm usually not concerned, but turning the effectively inline macros into two function calls seems excessive ;)
Also, it would save us another file.
-
Nico Huber (Code Review)