8a17983:

fa67b67 can be relevant.

Nice find! The commit message states "However, it requires exactly specifying the array size, which can be
cumbersome," but I don't think this is true.

Using the test program:

// repro.c
#include <stdio.h>

int main() {
    char str[] = "hello world";  // This should trigger the warning
    printf("%s\n", str);
    return 0;
}

I am able to compile with gcc14:

nix-shell --expr 'with import <nixpkgs> {}; mkShell.override { stdenv = overrideCC stdenv gcc14; }'
gcc -v
gcc -Wall -Wextra -Wpedantic -Werror repro.c -o out

and able to compile with gcc15:

nix-shell --expr 'with import <nixpkgs> {}; mkShell.override { stdenv = overrideCC stdenv gcc15; }'
gcc -v
gcc -Wall -Wextra -Wpedantic -Werror repro.c -o out

However, if I specify the array size, I can reproduce the error:

// repro.c
#include <stdio.h>

int main() {
    char str[11] = "hello world";  // This should trigger the warning
    printf("%s\n", str);
    return 0;
}

No error with:

nix-shell --expr 'with import <nixpkgs> {}; mkShell.override { stdenv = overrideCC stdenv gcc14; }'
gcc -Wall -Wextra -Wpedantic -Werror repro.c -o out

And an error with:

nix-shell --expr 'with import <nixpkgs> {}; mkShell.override { stdenv = overrideCC stdenv gcc15; }'
gcc -Wall -Wextra -Wpedantic -Werror repro.c -o out

repro.c: In function ‘main’:
repro.c:4:20: error: initializer-string for array of ‘char’ truncates NUL terminator but destination lacks ‘nonstring’ attribute (12 chars into 11 available) [-Werror=unterminated-string-initialization]
    4 |     char str[11] = "hello world";  // This should trigger the warning
      |                    ^~~~~~~~~~~~~
cc1: all warnings being treated as errors

Based on the above, I'd recommend we prefer the approach in this PR of not specifying the array size and perhaps document it as the preferred convention going forward? I find being able to specify the tag as a string to be much more reviewable than specifying the tag as an array of characters.

That being said, also happy to go the other way and update the musig tests to match the other modules if thats the preferred convention, as I think the main benefit is to have all of the modules follow the same convention.

To convince myself, I also verified with a few versions of clang, e.g.,:

nix-shell --expr 'with import <nixpkgs> {}; mkShell.override { stdenv = llvmPackages_16.stdenv; }'
clang -Wall -Wextra -Wpedantic -Werror -Wmost repro.c

@josibake The NUL byte resulting from char str[] = "hello world" does not hurt per se, but there are two minor issues with this: First, it's conceptually the wrong thing: If we want a char array, the simplest thing to do is to define a char array instead of a NUL-terminated string. Second and probably more relevant, it changes sizeof(str) to be 12 instead of 11. (See https://godbolt.org/z/da6PExKTh for demonstration. godbolt.org is the easiest way to test toy examples on many compilers.) We could, of course, accept this and always use sizeof(str) - 1, but it's easy to miss this.

edit: Sorry, I now saw that you're aware of the - 1 thing. And I agree, the ability to grep for the string is a good argument for the NUL-terminated string. If you ask me, I prefer to forego the grepability and define the right kind of object and have sizeof correct. But there's no definitive answer in the end.

@real-or-random thanks for the context! That explains the sizeof(str) - 1 for the musig examples. So it seems the choices are:

1. Do something conceptually wrong for something that is slightly easier to review
2. Do the conceptually correct thing for something that is slightly harder to review

"Slightly harder/easier" is a bit hand-wavy, but the fact that we used to specify the tags as strings (and the recently added musig also adopted this convention vs staying consistent with the existing modules) indicates option 1 is the more natural option. However, it likely needs an explainer, especially for why we are using sizeof(tag) - 1. On the flipside, I'm guessing option 2 feels more natural for reviewers who review/write a majority of the time in C?

Regardless of which convention is chosen, I do think its worth documenting in CONTRIBUTING.md. I'll add a commit for that once reviewers have weighed in on which convention they prefer.

Maybe godbolt.org/z/eKbT6sha4?

That still generates a warning if I add -Wextra.

Maybe godbolt.org/z/eKbT6sha4?

That still generates a warning if I add -Wextra.

Right.

https://godbolt.org/z/n5rf5Y7cP

Oh, interesting, I wasn't aware of nonstring. That's another neat way.

Though when I think about it, I still prefer {'h', 'e', 'l', 'l', 'o', ' ', 'w', 'o', 'r', 'l', 'd'}. Code is read much more often than it's written, so it makes sense to optimize reader (or reviewer) burden, and {'h', 'e', 'l', 'l', 'o', ' ', 'w', 'o', 'r', 'l', 'd'} is immediately clear to a reviewer familiar with C. It's just a bit hard on the eyes, but there will be no need to look up macros or GNU extension attributes, etc.

Though when I think about it, I still prefer {'h', 'e', 'l', 'l', 'o', ' ', 'w', 'o', 'r', 'l', 'd'}. Code is read much more often than it's written, so it makes sense to optimize reader (or reviewer) burden, and {'h', 'e', 'l', 'l', 'o', ' ', 'w', 'o', 'r', 'l', 'd'} is immediately clear to a reviewer familiar with C. It's just a bit hard on the eyes, but there will be no need to look up macros or GNU extension attributes, etc.

Agreed. That's why I raised this point in the first place.

Sounds like 2 votes for keeping it as is, vs one vote to change it 😅 I'll update this PR tomorrow to instead convert the musig module to the existing convention, and add a note documenting the convention.