Messages in this thread | | | From | "Huang\, Ying" <> | Subject | Re: [PATCH] llist: Put parentheses around parameters of llist_for_each_entry_safe() | Date | Tue, 26 Sep 2017 16:14:16 +0800 |
| |
"박병철/선임연구원/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
| |