[PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[Byungchul Park]

It would be somewhat safer to put parentheses around parameters of
a macro with parameters. Put it.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/llist.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index 1957635..e280b297 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head *list)
  * reverse the order by yourself before traversing.
  */
 #define llist_for_each_entry_safe(pos, n, node, member)			       \
-	for (pos = llist_entry((node), typeof(*pos), member);		       \
+	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
 	     member_address_is_nonnull(pos, member) &&			       \
-	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
-	     pos = n)
+	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
+	     (pos) = (n))
 
 /**
  * llist_empty - tests whether a lock-less list is empty
-- 
1.9.1

Re: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[Huang, Ying]

Hi, Byungchul,

Byungchul Park <byungchul.park@lge.com> writes:

> It would be somewhat safer to put parentheses around parameters of
> a macro with parameters. Put it.
>
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  include/linux/llist.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/llist.h b/include/linux/llist.h
> index 1957635..e280b297 100644
> --- a/include/linux/llist.h
> +++ b/include/linux/llist.h
> @@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head *list)
>   * reverse the order by yourself before traversing.
>   */
>  #define llist_for_each_entry_safe(pos, n, node, member)			       \
> -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> +	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
>  	     member_address_is_nonnull(pos, member) &&			       \
> -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> -	     pos = n)
> +	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
> +	     (pos) = (n))
>  
>  /**
>   * llist_empty - tests whether a lock-less list is empty

The original code follows the style of list_for_each_entry_safe().  The
parameters "pos" and "n" must be variable.  Because list_xxx family
functions work well so far, I think we needn't to change it too.

Best Regards,
Huang, Ying

RE: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)]

> -----Original Message-----
> From: Huang, Ying [mailto:ying.huang@intel.com]
> Sent: Tuesday, September 26, 2017 4:02 PM
> To: Byungchul Park
> Cc: peterz@infradead.org; mingo@kernel.org; linux-kernel@vger.kernel.org;
> kernel-team@lge.com; ying.huang@intel.com
> Subject: Re: [PATCH] llist: Put parentheses around parameters of
> llist_for_each_entry_safe()
> 
> Hi, Byungchul,
> 
> Byungchul Park <byungchul.park@lge.com> writes:
> 
> > It would be somewhat safer to put parentheses around parameters of
> > a macro with parameters. Put it.
> >
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  include/linux/llist.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/llist.h b/include/linux/llist.h
> > index 1957635..e280b297 100644
> > --- a/include/linux/llist.h
> > +++ b/include/linux/llist.h
> > @@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head *list)
> >   * reverse the order by yourself before traversing.
> >   */
> >  #define llist_for_each_entry_safe(pos, n, node, member)
> 	       \
> > -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> > +	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
> >  	     member_address_is_nonnull(pos, member) &&
> 	       \
> > -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> > -	     pos = n)
> > +	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true);
> \
> > +	     (pos) = (n))
> >
> >  /**
> >   * llist_empty - tests whether a lock-less list is empty
> 
> The original code follows the style of list_for_each_entry_safe().  The

Hello Huang,

I don’t see what you say here exactly, but let me note that all llist macros
are safe except the llist_for_each_entry_safe().

> parameters "pos" and "n" must be variable.  Because list_xxx family
> functions work well so far, I think we needn't to change it too.

I see. I don't want to argue much wrt such a trivial thing but I think
it would be better to fix it since the fix is fairly simple and clear. 
However, it's ok if the fix introduces a bad thing at least.

> Best Regards,
> Huang, Ying

Re: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[Huang, Ying]

"박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)"
<byungchul.park@lge.com> writes:

>> -----Original Message-----
>> From: Huang, Ying [mailto:ying.huang@intel.com]
>> Sent: Tuesday, September 26, 2017 4:02 PM
>> To: Byungchul Park
>> Cc: peterz@infradead.org; mingo@kernel.org; linux-kernel@vger.kernel.org;
>> kernel-team@lge.com; ying.huang@intel.com
>> Subject: Re: [PATCH] llist: Put parentheses around parameters of
>> llist_for_each_entry_safe()
>> 
>> Hi, Byungchul,
>> 
>> Byungchul Park <byungchul.park@lge.com> writes:
>> 
>> > It would be somewhat safer to put parentheses around parameters of
>> > a macro with parameters. Put it.
>> >
>> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
>> > ---
>> >  include/linux/llist.h | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/include/linux/llist.h b/include/linux/llist.h
>> > index 1957635..e280b297 100644
>> > --- a/include/linux/llist.h
>> > +++ b/include/linux/llist.h
>> > @@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head *list)
>> >   * reverse the order by yourself before traversing.
>> >   */
>> >  #define llist_for_each_entry_safe(pos, n, node, member)
>> 	       \
>> > -	for (pos = llist_entry((node), typeof(*pos), member);		       \
>> > +	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
>> >  	     member_address_is_nonnull(pos, member) &&
>> 	       \
>> > -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
>> > -	     pos = n)
>> > +	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true);
>> \
>> > +	     (pos) = (n))
>> >
>> >  /**
>> >   * llist_empty - tests whether a lock-less list is empty
>> 
>> The original code follows the style of list_for_each_entry_safe().  The
>
> Hello Huang,
>
> I don’t see what you say here exactly, but let me note that all llist macros
> are safe except the llist_for_each_entry_safe().
>
>> parameters "pos" and "n" must be variable.  Because list_xxx family
>> functions work well so far, I think we needn't to change it too.
>
> I see. I don't want to argue much wrt such a trivial thing but I think
> it would be better to fix it since the fix is fairly simple and clear. 
> However, it's ok if the fix introduces a bad thing at least.

Yes, it's simple.  But I don't think it helps too.  Considering that
list family functions with same style have no issues.

Best Regards,
Huang, Ying

RE: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)]

> -----Original Message-----
> From: Huang, Ying [mailto:ying.huang@intel.com]
> Sent: Tuesday, September 26, 2017 5:14 PM
> To: 박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)
> Cc: Huang, Ying; peterz@infradead.org; mingo@kernel.org; linux-
> kernel@vger.kernel.org; kernel-team@lge.com
> Subject: Re: [PATCH] llist: Put parentheses around parameters of
> llist_for_each_entry_safe()
> 
> "박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)"
> <byungchul.park@lge.com> writes:
> 
> >> -----Original Message-----
> >> From: Huang, Ying [mailto:ying.huang@intel.com]
> >> Sent: Tuesday, September 26, 2017 4:02 PM
> >> To: Byungchul Park
> >> Cc: peterz@infradead.org; mingo@kernel.org; linux-kernel@vger.kernel.org;
> >> kernel-team@lge.com; ying.huang@intel.com
> >> Subject: Re: [PATCH] llist: Put parentheses around parameters of
> >> llist_for_each_entry_safe()
> >>
> >> Hi, Byungchul,
> >>
> >> Byungchul Park <byungchul.park@lge.com> writes:
> >>
> >> > It would be somewhat safer to put parentheses around parameters of
> >> > a macro with parameters. Put it.
> >> >
> >> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> >> > ---
> >> >  include/linux/llist.h | 6 +++---
> >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/include/linux/llist.h b/include/linux/llist.h
> >> > index 1957635..e280b297 100644
> >> > --- a/include/linux/llist.h
> >> > +++ b/include/linux/llist.h
> >> > @@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head
> *list)
> >> >   * reverse the order by yourself before traversing.
> >> >   */
> >> >  #define llist_for_each_entry_safe(pos, n, node, member)
> >> 	       \
> >> > -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> >> > +	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
> >> >  	     member_address_is_nonnull(pos, member) &&
> >> 	       \
> >> > -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> >> > -	     pos = n)
> >> > +	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true);
> >> \
> >> > +	     (pos) = (n))
> >> >
> >> >  /**
> >> >   * llist_empty - tests whether a lock-less list is empty
> >>
> >> The original code follows the style of list_for_each_entry_safe().  The
> >
> > Hello Huang,
> >
> > I don’t see what you say here exactly, but let me note that all llist macros
> > are safe except the llist_for_each_entry_safe().
> >
> >> parameters "pos" and "n" must be variable.  Because list_xxx family
> >> functions work well so far, I think we needn't to change it too.
> >
> > I see. I don't want to argue much wrt such a trivial thing but I think
> > it would be better to fix it since the fix is fairly simple and clear.
> > However, it's ok if the fix introduces a bad thing at least.
> 
> Yes, it's simple.  But I don't think it helps too.  Considering that
> list family functions with same style have no issues.

In addition, making all the llist macros implementation consistent is
also meaningful, isn't it?

Ok.

I think your opinion is right and mine is also meaningful. I will just
follow the majority opinion.

Thank you for giving an opinion.

Re: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe()

[Byungchul Park]

On Tue, Sep 26, 2017 at 03:54:51PM +0900, Byungchul Park wrote:
> It would be somewhat safer to put parentheses around parameters of
> a macro with parameters. Put it.

Hello Ingo and Peter,

Even though Huang said this change is unnecessary, I'm curious about
what you think about this fix.

As Huang said, original code is safe unless users of llist use the
API weirdly, but I think this patch anyway makes the API safer and
align with the other llist APIs.

--
Thanks,
Byungchul

> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  include/linux/llist.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/llist.h b/include/linux/llist.h
> index 1957635..e280b297 100644
> --- a/include/linux/llist.h
> +++ b/include/linux/llist.h
> @@ -183,10 +183,10 @@ static inline void init_llist_head(struct llist_head *list)
>   * reverse the order by yourself before traversing.
>   */
>  #define llist_for_each_entry_safe(pos, n, node, member)			       \
> -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> +	for ((pos) = llist_entry((node), typeof(*(pos)), member);		       \
>  	     member_address_is_nonnull(pos, member) &&			       \
> -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> -	     pos = n)
> +	        ((n) = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
> +	     (pos) = (n))
>  
>  /**
>   * llist_empty - tests whether a lock-less list is empty
> -- 
> 1.9.1