Hi,
I've used checkpatch.pl to verify one of my patches. It complains:
ERROR: trailing statements should be on next line
#167: FILE: fs/quota/quota_tree.c:249:
+ for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
[...]
i++, ddquot += info->dqi_entry_size);
But the code looks like:
for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
i++, ddquot += info->dqi_entry_size);
Which is IMHO correct. Maybe it's because the for has actually empty body
and the ; is at the end of the line with for. But I didn't find anything in
CodingStyle that would forbid
for (...);
and
for (...)
;
Looks a bit strange.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Jan 27, 2009 at 04:49:05PM +0100, Jan Kara wrote:
> Hi,
>
> I've used checkpatch.pl to verify one of my patches. It complains:
>
> ERROR: trailing statements should be on next line
> #167: FILE: fs/quota/quota_tree.c:249:
> + for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> [...]
> i++, ddquot += info->dqi_entry_size);
>
> But the code looks like:
> for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
> i++, ddquot += info->dqi_entry_size);
>
> Which is IMHO correct. Maybe it's because the for has actually empty body
> and the ; is at the end of the line with for. But I didn't find anything in
> CodingStyle that would forbid
> for (...);
> and
> for (...)
> ;
> Looks a bit strange.
for (...); is a common C programming error, usually it's some kind of:
for(........);
do_something();
This code does something different than intended.
And yes, we had such bugs in the kernel.
for(........)
;
is correct. The "looks a bit strange" is what actually tells readers
what the code is doing (and that the author did it intentionally).
> Honza
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Tue, 27 Jan 2009 16:49:05 +0100
Jan Kara <[email protected]> wrote:
> Hi,
>
> I've used checkpatch.pl to verify one of my patches. It complains:
>
> ERROR: trailing statements should be on next line
> #167: FILE: fs/quota/quota_tree.c:249:
> + for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> [...]
> i++, ddquot += info->dqi_entry_size);
>
> But the code looks like:
> for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> i < qtree_dqstr_in_blk(info)
> && !qtree_entry_unused(info, ddquot); i++, ddquot +=
> info->dqi_entry_size);
>
while tihs might be correct C... don't you think it would be much
better to actually have a statement here rather than cramming
everything into the for ?
On Tue 27-01-09 08:19:54, Arjan van de Ven wrote:
> On Tue, 27 Jan 2009 16:49:05 +0100
> Jan Kara <[email protected]> wrote:
>
> > Hi,
> >
> > I've used checkpatch.pl to verify one of my patches. It complains:
> >
> > ERROR: trailing statements should be on next line
> > #167: FILE: fs/quota/quota_tree.c:249:
> > + for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > [...]
> > i++, ddquot += info->dqi_entry_size);
> >
> > But the code looks like:
> > for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > i < qtree_dqstr_in_blk(info)
> > && !qtree_entry_unused(info, ddquot); i++, ddquot +=
> > info->dqi_entry_size);
> >
>
> while tihs might be correct C... don't you think it would be much
> better to actually have a statement here rather than cramming
> everything into the for ?
This is an old code and I was just wrapping lines to fit into 80 chars...
But you're right, I can rewrite the loop into more readable form when I'm
at it.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue 27-01-09 18:06:01, Adrian Bunk wrote:
> On Tue, Jan 27, 2009 at 04:49:05PM +0100, Jan Kara wrote:
> > Hi,
> >
> > I've used checkpatch.pl to verify one of my patches. It complains:
> >
> > ERROR: trailing statements should be on next line
> > #167: FILE: fs/quota/quota_tree.c:249:
> > + for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > [...]
> > i++, ddquot += info->dqi_entry_size);
> >
> > But the code looks like:
> > for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
> > i++, ddquot += info->dqi_entry_size);
> >
> > Which is IMHO correct. Maybe it's because the for has actually empty body
> > and the ; is at the end of the line with for. But I didn't find anything in
> > CodingStyle that would forbid
> > for (...);
> > and
> > for (...)
> > ;
> > Looks a bit strange.
>
> for (...); is a common C programming error, usually it's some kind of:
>
> for(........);
> do_something();
>
> This code does something different than intended.
> And yes, we had such bugs in the kernel.
>
>
> for(........)
> ;
>
> is correct. The "looks a bit strange" is what actually tells readers
> what the code is doing (and that the author did it intentionally).
OK, makes some sence. Thanks for explanation.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Jan 27, 2009 at 06:06:01PM +0200, Adrian Bunk wrote:
> On Tue, Jan 27, 2009 at 04:49:05PM +0100, Jan Kara wrote:
> > Hi,
> >
> > I've used checkpatch.pl to verify one of my patches. It complains:
> >
> > ERROR: trailing statements should be on next line
> > #167: FILE: fs/quota/quota_tree.c:249:
> > + for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > [...]
> > i++, ddquot += info->dqi_entry_size);
> >
> > But the code looks like:
> > for (i = 0, ddquot = buf + sizeof(struct qt_disk_dqdbheader);
> > i < qtree_dqstr_in_blk(info) && !qtree_entry_unused(info, ddquot);
> > i++, ddquot += info->dqi_entry_size);
> >
> > Which is IMHO correct. Maybe it's because the for has actually empty body
> > and the ; is at the end of the line with for. But I didn't find anything in
> > CodingStyle that would forbid
> > for (...);
> > and
> > for (...)
> > ;
> > Looks a bit strange.
>
> for (...); is a common C programming error, usually it's some kind of:
>
> for(........);
> do_something();
>
> This code does something different than intended.
> And yes, we had such bugs in the kernel.
>
>
> for(........)
> ;
>
> is correct. The "looks a bit strange" is what actually tells readers
> what the code is doing (and that the author did it intentionally).
Yeah its about being explicit that you intended there to be an empty
statement in this construct. We tend to get bitten even more by if form
of this:
if (foo);
something();
But we catch them all. And always remember if you really think it looks
better or makes more sense one way and checkpatch is upset you can
ignore checkpatch. It is advice on what will be accepted not
necessarily the final arbiter. That is the maintainers role, checkpatch
is a tool to help you.
-apw