[flashrom] [RFC] Coding style limits

[Carl-Daniel Hailfinger]

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."