On 17.12.2009 11:10, Uwe Hermann wrote:
On Wed, Dec 16, 2009 at 09:23:44AM +0100, Luc Verhaegen wrote:
not religiously stuck to it, as was witnessed in the board enable table discussion.
I prefer sticking to 80 chars where it does not hurt, and it should be trivially possible to stick to 80 chars in most cases. Not sure what
I fully agree.
Yes, most cases are no problem. I am concerned with those where we have to break up strings excessively.
exact line causes this now, but i am sure that it can be solved: strings can be broken up trivially, and we should all by now be used to broken up strings.
Yep.
Also, as the coding style document we follow (the Linux one) states very nicely, if you need more that 80 chars per line, that's usually a good indicator that the code is nested too much and should be refactored. I fully agree with that statement.
Example from an upcoming patch.
@@ -743,19 +805,36 @@ int ret = 0; int oppos, preoppos; for (; (cmds->writecnt || cmds->readcnt) && !ret; cmds++) { - /* Is the next command valid or a terminator? */ if ((cmds + 1)->writecnt || (cmds + 1)->readcnt) { + /* Next command is valid. */ preoppos = find_preop(curopcodes, cmds->writearr[0]); oppos = find_opcode(curopcodes, (cmds + 1)->writearr[0]); - /* Is the opcode of the current command listed in the - * ICH struct OPCODES as associated preopcode for the - * opcode of the next command? + if ((oppos != -1) && (preoppos != -1)) { + /* Current command is listed as preopcode in + * ICH struct OPCODES and next command is listed + * as opcode in that struct. + */ + if ((curopcodes->opcode[oppos].atomic - 1) == + preoppos) { + /* They are paired up. */ + continue; + } else { + /* FIXME: We should simply adjust the + * pairing automatically. + */ + printf_debug("%s: preop 0x%02x and op " + "0x%02x are not paired up," + " executing as separate " + "ops\n", __func__, + cmds->writearr[0], + (cmds + 1)->writearr[0]); + } + } + /* FIXME: Check if this is a preop and the next command + * is an unknown op. In that case, reprogram opcode menu + * if possible. */ - if ((oppos != -1) && (preoppos != -1) && - ((curopcodes->opcode[oppos].atomic - 1) == preoppos)) - continue; - } - + } ret = ich_spi_send_command(cmds->writecnt, cmds->readcnt, cmds->writearr, cmds->readarr); }
Splitting the function above into multiple functions really doesn't make any sense, and the innermost printf_debug is really not funny anymore.
The patch hunk above was my motivator for the coding style RFC. Suggestions how to solve it are welcome.
Of course there are exceptions (overly long printf's that cannot be wrapped nicely, or the board-enable table), that's fine. We can live with a few exceptions that make sense. But as a general rule I'm strongly in favor of sticking to 80 chars.
I also wouldn't object a patch which makes our prints shorter, e.g. changing
printf_debug("Foo");
or
printf(MSG_DEBUG, "Foo");
to
printk(DBG, "Foo")
printk(DBG, "Foo") saves 1 character compared to printf_debug("Foo"), so it isn't the best choice.
or
// d for DEBUG, i for INFO, e for ERROR printe("Foo"); printi("Foo"); printd("Foo");
or something like that (just an example, maybe someone can come up with a good name).
Using msg_dbg saves 5 chars over printf_debug and it is only 1 char longer than printe/printi/printd.
Independent of the coding style issue I think using msg_dbg for debug msg_err for error msg_inf for info might be a viable solution for length reduction of prints.
Regards, Carl-Daniel