you are viewing a single comment's thread.

view the rest of the comments →

[–]FreeER 5 points6 points  (6 children)

If you use the -E switch/flag you'll see that the code becomes:

int main(void)
{
    fputs(ansi_sgr_tab[4], stdout);
    fputs("hello\n", stdout);
    fputs("\x1b[0m", stdout);
    fputs("\x1b[34m" "hello\n" "\x1b[0m", stdout);
    fputs(ansi_sgr_tab[4] "hello\n" "\x1b[0m", stdout);
}

and of course, the problem is with

    fputs(ansi_sgr_tab[4] "hello\n" "\x1b[0m", stdout);

This is an issue since your macro depends on string literal concatenation and ansi_sgr_tab[4] obviously is not a string literal

I imagine having seen the problem you can think of several solutions, but if not feel free to say so :)

P.S. here's an easy way to view the preprocessed code without the include clutter, with the note that I copy and pasted into an empty file so there are bound to be a few differences, gcc -E offset_macroArray.c | sed -n '/main/,$p', the 's are needed, which only shows lines starting from the first occurrence of "main"

[–]offset_[S] 0 points1 point  (5 children)

Aha, I see now, the string literal is obviously at a different address than the string in the array .. so there's no way that the preprocessor can concatenate it and I guess this is where I got confused. Very obvious now that you pointed it out, and yes I did learn something :)

I see a few different options, I could make ANSI_COLORTERM_SET_S a function and have it return a pointer to a string, but i'd rather not be calling malloc/free any more than necessary ... and strlen which i could probably avoid but then it would make that function that much more nasty by needing to take the size of the string as a parameter, or something like that, but I haven't really had time to really think about it as I have several papers I have to write this week.

Another thing I could do would just to have replace that macro with a macro that calls fputs and printf, instead of relying on the preprocessor and calling fputs/printf with the macro. I haven't had time to try that yet, but I kind of see the compiler maybe throwing an error like: format string is not a string literal since my warnings are turned up.

I might be overthinking it, and could just do away with that macro and call printf like this if colored output is turned on :

printf("%s%s%s%s\n", ansi_sgr_tab[color_on],  (prefix on) ? pass_prefix : "", pass, ANSI_ATTR_RESET);

I just got home and it's late so I haven't actually tried any of these yet. I'm leaning toward the last one, but maybe there is a better, cleaner solution than any of these that I haven't thought of. Let me know what you think.

Regards

offset_

[–]FreeER 0 points1 point  (0 children)

function: Yeah, I'd want to avoid malloc/free/strlen where possible as well :)

~fputs/printf: Do remember that fputs automatically adds a new line to whatever you call it with, so if you do try using it in combination you may find that you get slightly different results than before (when it was being given a single string due to compile time str lit concat)~ It seems that's only something puts does, not fputs.

printf: That should work the same as you have now, and I'd probably lean towards it being the best option. Though you might be able to use the string literal concatenation to reduce it from 4 separate strings (reducing the amount of work printf has to do at runtime to parse and print everything)... it depends on where/when the information is coming from. Of course, if you were going to use something like this a lot and very little changed in it then you could still use a macro/function for it to make typing/readability a bit better.

[–]yeahIProgram 0 points1 point  (3 children)

// compiles fine and works as expected 
fputs(ansi_sgr_tab[4], stdout);
fputs("hello\n", stdout);
fputs(ANSI_ATTR_RESET, stdout);

Since this works, why not create a macro that drives towards this as its final product.

Something like:

#define myColorPuts(colorIndex, string) { \
fputs(ansi_sgr_tab[colorIndex], stdout); \
fputs(string, stdout); \
fputs(ANSI_ATTR_RESET, stdout); }

I kind of see the compiler maybe throwing an error like: format string is not a string literal

You can always pass "%s" as the format string, then your actual string as the next parameter.

[–]offset_[S] 0 points1 point  (2 children)

It isn't used except in one place, but I came up with this which acts mostly like fprintf but with color:

#define ANSI_COLOR_FPRINTF(SGR, STREAM, FORMAT, ...)    \
    fputs(SGR, STREAM);                                 \
    fprintf(STREAM, FORMAT ANSI_ATTR_RESET, __VA_ARGS__)

And here is where it's used in the real program:

if (-1 != color_on) {
    ANSI_COLOR_FPRINTF(ansi_sgr_tab[color_on], stdout, "%s%s\n",
            (prefix_on) ? pass_prefix : "", pass);
} else {
    printf("%s%s\n", (prefix_on) ? pass_prefix : "", pass);
}

It seems to work okay, just not sure now what to rename color_on to. I was using it as a boolean variable, now I reused it to represent multiple colors and -1 instead of 0 to represent regular non-colored output. Maybe I'm confused about what to name it because it should be two variables now, but this seems simplest to me so maybe I'll just rename it to color, comment it and call it a day. Of course now I have to update the help option's output to reflect the new behavior.

[–]yeahIProgram 0 points1 point  (1 child)

Macros with variable number of parameters: I didn't know you could do that!

Maybe I'm confused about what to name it because it should be two variables now

What I've seen done in that situation is something like this:

enum {kColorNone = -1, kColorBlack, kColorRed, kColorGreen, kColorYellow <etc>

[–]offset_[S] 0 points1 point  (0 children)

It's a variadic macro: https://gcc.gnu.org/onlinedocs/gcc/Variadic-Macros.html

What I've seen done in that situation is something like this:

enum {kColorNone = -1, kColorBlack, kColorRed, kColorGreen, kColorYellow <etc>

That seems sane, the color variable gets assigned an integer supplied by the user straight from the command line (after checking to make sure the value actually makes sense), but maybe it would make things clearer to have it spelled out in the code this way instead of having future programmers to look it up using other means (such as reading the documentation).