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
--
Developer quote of the month:
"We are juggling too many chainsaws and flaming arrows and tigers."
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
