On Sat, Oct 27, 2018 at 11:56:54PM +0900, Akira Yokosawa wrote: > On 2018/10/26 08:58:30 +0800, Junchang Wang wrote: > [...] > > > > BTW, I found I'm not good in writing C macro (e.g., cmpxchg). Do you > > know some specification/document on writing C macro functions in > > Linux? > > Although I'm not qualified as a kernel developer, > Linux kernel's "coding style" has a section on this. See: > > https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl > > In that regard, macros I added in commit b2acf6239a95 > ("count: Tweak counttorture.h to avoid segfault") do not meet > the style guide in a couple of ways: > > 1) Inline functions are preferable to macros resembling functions > 2) Macros with multiple statements should be enclosed in a do - while block > 3) ... > > Any idea for improving them is more than welcome! Let's see... #define cmpxchg(ptr, o, n) \ ({ \ typeof(*ptr) _____actual = (o); \ \ __atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \ __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) ? (o) : (o)+1; \ }) We cannot do #1 because cmpxchg() is type-generic, and thus cannot be implemented as a C function. (C++ could use templates, but we are not writing C++ here.) We cannot do #2 because cmpxchg() must return a value. Indentation is not perfect, but given the long names really cannot be improved all that much, if at all. However, we do have a problem, namely the multiple uses of "o", which would be very bad if "o" was an expression with side-effects. How about the following? #define cmpxchg(ptr, o, n) \ ({ \ typeof(*ptr) _____old = (o); \ typeof(*ptr) _____actual = _____old; \ \ __atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \ __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) ? _____old : _____old + 1; \ }) This still might have problems with signed integer overflow, but I am inclined to ignore that for the moment. Because paying attention to it results in something like this: #define cmpxchg(ptr, o, n) \ ({ \ typeof(*ptr) _____old = (o); \ typeof(*ptr) _____actual = _____old; \ \ __atomic_compare_exchange_n(ptr, (void *)&_____actual, (n), 1, \ __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) \ ? _____old \ : _____old > 0 ? _____old - 1; : _____old + 1; \ }) Thoughts? Most especially, any better ideas? Thanx, Paul
