On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote:
> Completely reasonable. Thanks.
If we're doing "completely reasonable" complaints, then ...
- I don't understand why plain 'unsigned' is deemed bad.
- The rule about all function parameters in prototypes having a name
doesn't make sense. Example:
int ida_get_new_above(struct ida *ida, int starting_id, int *p_id);
There is zero additional value in naming 'ida'. I know it's an ida.
The struct name tells me that. If there're two struct ida pointers
in the prototype, then sure, I want to name them so I know which is
which (maybe 'src' and 'dst'). Having an unadorned 'int' parameter
to a function should be a firable offence. But there's no need to
call 'gfp_t' anything. We know it's a gfp_t. Adding 'gfp_mask'
after it doesn't tell us anything extra.
- Forcing a blank line after variable declarations sometimes makes for
some weird-looking code. For example, there is no problem with this
code (from a checkpatch PoV):
if (xa_is_sibling(entry)) {
offset = xa_to_sibling(entry);
entry = xa_entry(xas->xa, node, offset);
/* Move xa_index to the first index of this entry */
xas->xa_index = (((xas->xa_index >> node->shift) &
~XA_CHUNK_MASK) | offset) << node->shift;
}
but if I decide I don't need 'offset' outside this block, and I want
to move the declaration inside, it looks like this:
if (xa_is_sibling(entry)) {
unsigned int offset = xa_to_sibling(entry);
entry = xa_entry(xas->xa, node, offset);
/* Move xa_index to the first index of this entry */
xas->xa_index = (((xas->xa_index >> node->shift) &
~XA_CHUNK_MASK) | offset) << node->shift;
}
Does that blank line really add anything to your comprehension of the
block? It upsets my train of thought.
Constructively, I think this warning can be suppressed for blocks
that are under, say, 8 lines. Or maybe indented blocks is where I don't
want this warning. Not sure.
Here's another example where I don't think the blank line adds anything:
static inline int xa_store_empty(struct xarray *xa, unsigned long index,
void *entry, gfp_t gfp, int errno)
{
void *curr = xa_cmpxchg(xa, index, NULL, entry, gfp);
if (!curr)
return 0;
if (xa_is_err(curr))
return xa_err(curr);
return errno;
}
So line count definitely has something to do with it.
- There's no warning for the first paragraph of section 6:
6) Functions
------------
Functions should be short and sweet, and do just one thing. They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well.
I'm not expecting you to be able to write a perl script that checks
the first line, but we have way too many 200-plus line functions in
the kernel. I'd like a warning on anything over 200 lines (a factor
of 4 over Linus's stated goal).
- I don't understand the error for xa_head here:
struct xarray {
spinlock_t xa_lock;
gfp_t xa_flags;
void __rcu * xa_head;
};
Do people really think that:
struct xarray {
spinlock_t xa_lock;
gfp_t xa_flags;
void __rcu *xa_head;
};
is more aesthetically pleasing? And not just that, but it's an *error*
so the former is *RIGHT* and this is *WRONG*. And not just a matter
of taste?
