Re: [PATCH] Xen/atomic: use static inlines instead of macros

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/02/14 10:54, Jan Beulich wrote:
>>>> On 24.02.14 at 11:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 24/02/14 10:02, Jan Beulich wrote:
>>>>>> On 21.02.14 at 21:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> This is some coverity-inspired tidying.
>>>>
>>>> Coverity has some grief analysing the call sites of atomic_read().  This is
>>>> believed to be a bug in Coverity itself when expanding the nested macros, 
>>>> but
>>>> there is no legitimate reason for it to be a macro in the first place.
>>>>
>>>> This patch changes {,_}atomic_{read,set}() from being macros to being static
>>>> inline functions, thus gaining some type safety.
>>>>
>>>> One issue which is not immediatly obvious is that the non-atomic varients 
>>>> take
>>>> their atomic_t at a different level of indirection to the atomic varients.
>>>>
>>>> This is not suitable for _atomic_set() (when used to initialise an atomic_t)
>>>> which is converted to take its parameter as a pointer.  One callsite of
>>>> _atomic_set() is updated, while the other two callsites are updated to
>>>> ATOMIC_INIT().
>>> Did you consider leaving these "non-atomic atomic ops" untouched
>>> (as they don't involve macro nesting), altering only the "real" ones?
>> Yes, but for the sake of three updates at callsites, I felt the benefits
>> outweighed the costs.
> Except that I don't really see much of a benefit here - the type safety
> argument doesn't really count all that much, considering that a wrongly
> used type would need to have a suitable field named "counter", which
> is unlikely enough to not worry much.
>
> Jan
>

An error message of "Expeted atomic_t *, got <something else>" is
substantially more useful than "<something> doesn't have .counter" or .
being an invalid operator in context.

~Andrew