Yes, I guess a single patch is indeed OK. I have few nit-picks, though.
On Tue, 2014-09-30 at 18:13 +0300, Tanya Brokhman wrote:
> - ubi_err("'ubi_io_read_ec_hdr()' returned unknown code %d", err);
> + ubi_err(ubi,
> + "'ubi_io_read_ec_hdr()' returned unknown code %d", err);
> return -EINVAL;
I think it is fine if the line is long in these cases, let's keep the
message on the same line, this split does not contribute to better
readability, quite the opposite, in my opinion.
One line:
ubi_err(ubi, "long line")
Multiple lines:
ubi_err(ubi, "long line,
parameters)
> - ubi_msg("\"delete\" compatible internal volume %d:%d found, will remove it",
> + ubi_msg(ubi,
> + "\"delete\" compatible internal volume %d:%d found, will remove it",
> vol_id, lnum);
Ditto.
> - ubi_msg("read-only compatible internal volume %d:%d found, switch to read-only mode",
> + ubi_msg(ubi,
> + "read-only compatible internal volume %d:%d found, switch to read-only mode",
> vol_id, lnum);
Ditto. And so on, in many places.
> - ubi_err("bad compat %d", vidh->compat);
> + ubi_err(ubi,
> + "bad compat %d", vidh->compat);
And for consistency, places like this would be:
ubi_err(ubi, bad compat %d",
vidh->compat);
> if (av->used_ebs != be32_to_cpu(vidh->used_ebs)) {
> - ubi_err("bad used_ebs %d", av->used_ebs);
> + ubi_err(ubi,
> + "bad used_ebs %d", av->used_ebs);
Ditto for all the similar cases.
> - ubi_msg("volume %d (\"%s\") re-sized from %d to %d LEBs", vol_id,
> + ubi_msg(ubi, "volume %d (\"%s\") re-sized from %d to %d LEBs",
> + vol_id,
> vol->name, old_reserved_pebs, vol->reserved_pebs);
Please, in cases like this, try to pack more arguments to the second
line, and move those which do not fit there to the third line. So this
would be like:
ubi_msg(ubi, "volume %d (\"%s\") re-sized from %d to %d LEBs",
vol_id, vol->name, old_reserved_pebs,
vol->reserved_pebs);
On 3 October 2014 17:27, Artem Bityutskiy <[email protected]> wrote:
> Yes, I guess a single patch is indeed OK. I have few nit-picks, though.
>
> On Tue, 2014-09-30 at 18:13 +0300, Tanya Brokhman wrote:
>> - ubi_err("'ubi_io_read_ec_hdr()' returned unknown code %d", err);
>> + ubi_err(ubi,
>> + "'ubi_io_read_ec_hdr()' returned unknown code %d", err);
>> return -EINVAL;
>
> I think it is fine if the line is long in these cases, let's keep the
> message on the same line, this split does not contribute to better
> readability, quite the opposite, in my opinion.
>
> One line:
> ubi_err(ubi, "long line")
>
> Multiple lines:
> ubi_err(ubi, "long line,
> parameters)
You should discuss that with checkpatch team, because ARAIR it will
complain about "long line" with any other parameter in the same line.
On Fri, 2014-10-03 at 17:50 +0200, Rafał Miłecki wrote:
> On 3 October 2014 17:27, Artem Bityutskiy <[email protected]> wrote:
> > Yes, I guess a single patch is indeed OK. I have few nit-picks, though.
> >
> > On Tue, 2014-09-30 at 18:13 +0300, Tanya Brokhman wrote:
> >> - ubi_err("'ubi_io_read_ec_hdr()' returned unknown code %d", err);
> >> + ubi_err(ubi,
> >> + "'ubi_io_read_ec_hdr()' returned unknown code %d", err);
> >> return -EINVAL;
> >
> > I think it is fine if the line is long in these cases, let's keep the
> > message on the same line, this split does not contribute to better
> > readability, quite the opposite, in my opinion.
> >
> > One line:
> > ubi_err(ubi, "long line")
> >
> > Multiple lines:
> > ubi_err(ubi, "long line,
> > parameters)
>
> You should discuss that with checkpatch team, because ARAIR it will
> complain about "long line" with any other parameter in the same line.
I respect checkpatch.pl, and uniformity / consistency, but in this
particular case I put my maintainer hat on and say that for this kernel
subsystem it is fine, because the maintainer will be more efficient in
maintaining this code when the code is a bit mere readable for him.
Artem.
On 3 October 2014 18:19, Artem Bityutskiy <[email protected]> wrote:
> On Fri, 2014-10-03 at 17:50 +0200, Rafał Miłecki wrote:
>> On 3 October 2014 17:27, Artem Bityutskiy <[email protected]> wrote:
>> > Yes, I guess a single patch is indeed OK. I have few nit-picks, though.
>> >
>> > On Tue, 2014-09-30 at 18:13 +0300, Tanya Brokhman wrote:
>> >> - ubi_err("'ubi_io_read_ec_hdr()' returned unknown code %d", err);
>> >> + ubi_err(ubi,
>> >> + "'ubi_io_read_ec_hdr()' returned unknown code %d", err);
>> >> return -EINVAL;
>> >
>> > I think it is fine if the line is long in these cases, let's keep the
>> > message on the same line, this split does not contribute to better
>> > readability, quite the opposite, in my opinion.
>> >
>> > One line:
>> > ubi_err(ubi, "long line")
>> >
>> > Multiple lines:
>> > ubi_err(ubi, "long line,
>> > parameters)
>>
>> You should discuss that with checkpatch team, because ARAIR it will
>> complain about "long line" with any other parameter in the same line.
>
> I respect checkpatch.pl, and uniformity / consistency, but in this
> particular case I put my maintainer hat on and say that for this kernel
> subsystem it is fine, because the maintainer will be more efficient in
> maintaining this code when the code is a bit mere readable for him.
I'm fine with that :) I think it may be even worth bringing to the
checkpatch / CodingStyle to allow such lines.
--
Rafał
On Fri, 3 Oct 2014, Artem Bityutskiy wrote:
> I respect checkpatch.pl, and uniformity / consistency, but in this
> particular case I put my maintainer hat on and say that for this kernel
> subsystem it is fine, because the maintainer will be more efficient in
> maintaining this code when the code is a bit mere readable for him.
I believe checkpatch.pl flags long lines as a warning, not as an error,
indicating some relaxation on the severity to start with.
/Ricard
--
Ricard Wolf Wanderlöf ricardw(at)axis.com
Axis Communications AB, Lund, Sweden http://www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30