lkml.org 
[lkml]   [2016]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: checkpatch falsepositives in Lustre code
From
Date
On Mon, 2016-02-15 at 21:45 -0500, Oleg Drokin wrote:
> On Feb 15, 2016, at 9:27 PM, Joe Perches wrote:
>
> > On Mon, 2016-02-15 at 20:57 -0500, Oleg Drokin wrote:
> > > On Feb 15, 2016, at 7:56 PM, Joe Perches wrote:
> > > > [etc...]
> > > >
> > > > Yeah, that's a defect of some type.
> > >
> > > Also while I have your attention, here's another one:
> > >
> > > struct cfs_percpt_lock *
> > > cfs_percpt_lock_alloc(struct cfs_cpt_table *cptab)
> > > {
> > >         struct cfs_percpt_lock  *pcl;
> > >         spinlock_t              *lock;
> > >         int                     i;
> > > …
> > >         cfs_percpt_for_each(lock, i, pcl->pcl_locks)
> > >                 spin_lock_init(lock);
> > >
> > > The declaration of the spinlock pointer produces:
> > > CHECK: spinlock_t definition without comment
> > >
> > > Should spinlock pointers really be included in the check, it's obvious that
> > > they themselves are not really protecting anything, esp. considering it's a
> > > local function variable here.
> >
> > I don't have an opinion here.
> >
> > spinlock_t pointers are relatively rare.
>
> I guess they are. And I understand why you would want a comment for the actual
> spinlock, but pointexr - much less so.
>
> Anyway, I have some more questions:
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #8720: FILE: drivers/staging/lustre/lustre/libcfs/tracefile.h:189:
> +#define cfs_tcd_for_each(tcd, i, j)                                   \
> +       for (i = 0; cfs_trace_data[i]; i++)                             \
> +               for (j = 0, ((tcd) = &(*cfs_trace_data[i])[j].tcd);     \
> +                    j < num_possible_cpus();                            \
> +                    j++, (tcd) = &(*cfs_trace_data[i])[j].tcd)
>
> This is a macros with complex value alright, but the whole idea of this one
> is to not be enclosed. Any ideas about this one and similar?

checkpatch is brainless script and a not a real parser.
Ignoring its stupid and incorrect messages is a good idea.

fyi: There are many of these messages that exist like below.

I can't think of a reasonable way to automatically identify
and not show the defective error messages for these. Andy?

---

ERROR: Macros with complex values should be enclosed in parentheses
#86: FILE: include/linux/dmar.h:86:
+#define for_each_active_drhd_unit(drhd) \
+ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \
+ if (drhd->ignored) {} else

ERROR: Macros with complex values should be enclosed in parentheses
#90: FILE: include/linux/dmar.h:90:
+#define for_each_active_iommu(i, drhd) \
+ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \
+ if (i=drhd->iommu, drhd->ignored) {} else

ERROR: Macros with complex values should be enclosed in parentheses
#94: FILE: include/linux/dmar.h:94:
+#define for_each_iommu(i, drhd) \
+ list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \
+ if (i=drhd->iommu, 0) {} else 

ERROR: Macros with complex values should be enclosed in parentheses
#110: FILE: include/linux/dmar.h:110:
+#define for_each_active_dev_scope(a, c, p, d) \
+ for_each_dev_scope((a), (c), (p), (d)) if (!(d)) { continue; } else

total: 4 errors, 0 warnings, 285 lines checked

\
 
 \ /
  Last update: 2016-02-16 04:21    [W:0.056 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site