From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH v2] Xen/atomic: use static inlines instead of macros Date: Tue, 01 Apr 2014 11:39:22 +0100 Message-ID: <533A975A.1060402@gmail.com> References: <1396004108-27981-1-git-send-email-andrew.cooper3@citrix.com> <20140328112212.GA87844@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8738083481090831880==" Return-path: In-Reply-To: <20140328112212.GA87844@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: Keir Fraser , Ian Campbell , Andrew Cooper , Xen-devel , Stefano Stabellini , Jan Beulich List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============8738083481090831880== Content-Type: multipart/alternative; boundary="------------040507040504030804060502" This is a multi-part message in MIME format. --------------040507040504030804060502 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Tim Deegan wrote: > > At 10:55 +0000 on 28 Mar (1396000508), Andrew Cooper 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 immediately obvious is that the non-atomic >> variants take >> their atomic_t at a different level of indirection to the atomic >> variants. >> >> 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(). >> >> Signed-off-by: Andrew Cooper > > > Reviewed-by: Tim Deegan > > FWIW, I approve of converting both kinds of accessors to inlines, just > on general principles. Agreed, functions are preferable to macros. Acked-by: Keir Fraser --------------040507040504030804060502 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Tim Deegan wrote:

At 10:55 +0000 on 28 Mar (1396000508), Andrew Cooper 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 immediately obvious is that the non-atomic variants take
their atomic_t at a different level of indirection to the atomic variants.

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().

Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>


Reviewed-by: Tim Deegan<tim@xen.org>

FWIW, I approve of converting both kinds of accessors to inlines, just
on general principles.


Agreed, functions are preferable to macros.

Acked-by: Keir Fraser <keir@xen.org>
--------------040507040504030804060502-- --===============8738083481090831880== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8738083481090831880==--