On 02/09/2026 1:50 pm, Jan Beulich wrote: > On 02.09.2026 13:43, Andrew Cooper wrote: >> All this does is obfuscate the usage sites. > I don't mind these changes, yet I'd like to express that as so often there > are multiple ways of looking at things. The benefit of the macros was that > the use sites were textually shorter, and hence easier to process (assuming > you know what ADDR stands for).
"assuming" is the whole problem, and what makes this obfuscation. If it were a pattern we used everywhere, it would be fine, but it's unique to this header. It hides the width and volatility of the access, which variable is being referenced, and the indirection. The only thing it has going for it is that it is recognisable as "probably a macro", but you're still forced to look elsewhere to identify how it works. > >> --- a/xen/arch/x86/include/asm/bitops.h >> +++ b/xen/arch/x86/include/asm/bitops.h >> @@ -9,16 +9,6 @@ >> #include <asm/asm_defns.h> >> #include <asm/cpufeatureset.h> >> >> -/* >> - * We specify the memory operand as both input and output because the memory >> - * operand is both read from and written to. Since the operand is in fact a >> - * word array, we also specify "memory" in the clobbers list to indicate >> that >> - * words other than the one directly addressed by the memory operand may be >> - * modified. >> - */ > Might we not better keep this justification for the seemingly odd memory > clobbers? (The first sentence, otoh, I agree can be dropped.) I don't think it's useful to keep, but fine... ~Andrew
