did I send the file??? jeje guess not, here it goes

Rob Landley rob at landley.net
Fri Sep 16 03:10:42 UTC 2005


Reviewing the patch:

1) which still has an h in it. :)

case '1':
+      printf( "bold attribute on\n" );  
+      break;
+     case '2':
+      printf( "dark attribute on\n" );  
+      break;
+     case '4':
+      printf( "underline attribute on\n" );  
+      break;
+     case '5':
+      printf( "blink attribute on\n" ); 

The " attribute on\n" part of those strings are identical, it would be nice if 
this could feed into a printf("%s attribute on\n", blah); somehow...

Your code has trailing spaces after some of the lines I just quoted, and 
shouldn't.

+   switch (* ++backopt) {
+    case '0':
+     printf( "black\n" ); 
+     break;
+    case '1':
+     printf( "red\n" );  
+     break;
+    case '2':
+     printf( "green\n" );
+     break;
+    case '3':
+     printf( "yellow\n" );
+     break;
+    case '4':
+     printf( "blue\n" );
+     break;
+    case '5':
+     printf( "magenta\n" );
+     break;
+    case '6':
+     printf( "cyan\n" );
+     break;
+    case '7':
+     printf( "white\n" );
+     break;
+    }
+

How about reorganizing that to look more like printf("%s\n", 
color_array[*++backopt]);  (Possibly with an if statement in front of it or 
an &7 on backopt if values <0 or >7 are possible.)
 
Um, I thought you said you were yanking BB_SETTERM?

Your big comment at the end starting "this code was written from scrath(sic)" 
is nice for showing us your throught process, but don't check it in.  If it 
is indeed written from scratch, say so at the top above the line about the 
GPL.  (And not the full boilerplate please.  Since busybox includes the full 
text of the GPL in the tarball, we're just saying something like "This code 
is licensed under GPL v2, see LICENSE file for full text."

ANSI escape sequences are a standard, so if you're going to credit anything 
credit the console_codes man page.

Rob



More information about the busybox mailing list