Minimal colored write

I remembered a cwrite application that I used in the good old DOS times.

Basically it took a color code as first argument and some text as the second and printed the given text in the specified color. Also it supported some escape sequences.

Since Windows NT does not support ANSI Escape Codes it's not as easy to change the color of certain text within a batch script.

Well Windows 10 does support ANSI.... (It took MS 10 Versions to implement that?!)

Now I wanted to re-implement that cwrite from those days. With minimal requirements. Just win32 api. Nothing else.

I have commented some lines in the code that I am unsure of.

```
#include

int main(int argc, char ** argv) {
HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
if(!h) { return -2; } // god help us

CONSOLE_SCREEN_BUFFER_INFO csbiInfo;
BOOL v = GetConsoleScreenBufferInfo(h, &csbiInfo);
if(!v) { return -1; } // still... god should help us

WORD wOld = csbiInfo.wAttributes;

// No arguments: show the user how it works
// and how the colors look...
if(argc [text]\n", argv[0]);
printf(" can be any of the following:\n");
WORD i;

for(i = 0; i < 0x100; i++) {
v &= SetConsoleTextAttribute ( h, i );
if(i % 0x10 == 0) { printf("\n"); }
printf("%4i", i);

}
printf("\n");
v &= SetConsoleTextAttribute( h, wOld);
return v;
}

WORD wNew = (WORD)atoi(argv[1]);

v &= SetConsoleTextAttribute ( h, wNew );
// well now this is... unfortunate
if(!v) { return -3; }

/*
* Classic "it's not a bug, but a feature"
* If there is no argv[2] then we just change the color
* ... without resetting to the original
*/
if(argc < 3) { return 2; }

// Now this is to handle some special characters
// parhaps I should use sprintf?
WORD j;
for(j = 0; j < strlen(argv[2]); j++) {
if(argv[2][j] == '\\') {
switch(argv[2][j + 1]) {

A few things I noticed when reading through (I don't know much about how Windows.h works, so I mostly focused on stuff not related to it)

-
This made me laugh:

// god help us

At the same time, though, it doesn't really describe what's happening. In this case, you don't really need an explanation; it's decently obvious what's up just by reading the relevant documentation. In other cases, though, you don't explain. For example...

-
here:

// parhaps I should use sprintf?

Why aren't you? I'm assuming the answer is "I couldn't see a clear benefit", but then explain that in a few words. (also, *perhaps)

-
wOld and wNew are bad variable names. Something like oldAttributes and newAttributes would be better, since it describes what's actually going on. If that's not what's going on, you should provide better variable names so I can understand what they should be :)

-
Why are you making j a WORD? strlen returns a size_t; while WORD is (usually, IIRC) 16 bits, size_t can be a great deal larger. Is it unlikely that you'll be encountering strings that are longer than 2^16 characters? Yes. Is the fix so easy to implement that there's no reason not to? Also yes.

-
Possible UB at switch(argv[2][j + 1]). If j == strlen(argv[2]), argv[2][j + 1] accesses memory outside of argv[2] (one past the end, in fact). Depending on how, specifically, the memory is laid out, this may not crash the program. It'd be better to do for (j = 1; j

-
strlen operates by doing something roughly like this:

char *cur = your_string;
while (*cur) ++cur;
return cur - your_string;

That means that it's O(n). Performing that calculation makes your whole loop there O(n^2). If you store the length of argv[2], then compare to the stored length, you can improve the speed by a decent amount. Of course, for the number of characters you're most often dealing with, and with how simple your loop is, this doesn't matter so much, but it's a bad habit to get into.

-
Pretend it's six months later and you've been dealing exclusively not with text. When you see this:

argv[2][j]   = 0xa;
argv[2][j+1] = 0xd;

are you going to say "that's super clear and I know exactly what it does", or will you have to Google what the ASCII characters 0xa and 0xd are? You should put a comment there explaining it.

-
// How do I turn a char ** into a va_list ???

I don't think you can. Sorry.

-
In that same area, why are you assuming that accessing argv at argc and above will be valid, or that argv[2] will be a valid format string? You should check. Alternatively, just forget about building formatting in and let the calling code handle it (Bash, for example, can do that easily) -- it's a lot of work to try to do it yourself, for little to no benefit.

-
Your error codes are uninformative. Given that all you have is an integer, that's pretty much par for the course, but the least you could do is document them in the help message.

-
Related: Try running this simple C program from Bash:

int main() { return -1; }

Look at its return code -- it's 255, not -1`. See here for more information; the gist of it is that negative numbers might not be interpreted that way by the OS. Stick to positive ones.

-
What's the point of this?

if(!v) { /* nop */ } // could not reset color...

The compiler just optimizes it out anyway; you might as well replace it with

// if it fails, we don't really care

to make it blatantly obvious that it doesn't matter.

-
feature-request Let me put in a color's name and see the output in that color, rather than making me use the number.

That's all I saw. Someone who's more familiar with the Windows API might find more.

StackExchange Code Review Q#154935, answer score: 2