[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 24/02/14 10:54, Jan Beulich wrote:
>>>> On 24.02.14 at 11:26, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 24/02/14 10:02, Jan Beulich wrote:
>>>>>> On 21.02.14 at 21:41, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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
>>> 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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.