| Message ID | 20220710081135.74964-1-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-objects: fix compilation warning/error due to missing braces | expand |
On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing > braces around initialization of a subobject, which is problematic when > building with `DEVELOPER=YesPlease` which enables `-Werror`: > > builtin/unpack-objects.c:388:26: error: suggest braces around > initialization of subobject [-Werror,-Wmissing-braces] > git_zstream zstream = { 0 }; > > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)" > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > > Notes: > This is atop 'hx/unpack-streaming' which is already in 'next'. > All the CI builds are fine with this change. > > As I understand it, this should be a safe change; the fields which > follow `z_stream z` in `git_zstream` will be initialized to zero > since the first field has an explicit initializer. > > builtin/unpack-objects.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c > index 43789b8ef2..c606c92e37 100644 > --- a/builtin/unpack-objects.c > +++ b/builtin/unpack-objects.c > @@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream, > > static void stream_blob(unsigned long size, unsigned nr) > { > - git_zstream zstream = { 0 }; > + git_zstream zstream = {{ 0 }}; > struct input_zstream_data data = { 0 }; > struct input_stream in_stream = { > .read = feed_input_zstream, > -- > 2.37.0.236.gcef32db0b6.dirty > Not a comment, just wondering, when should I use "{ { 0 } }" and when should I use "{ 0 }"? I didn't get the error with "Apple clang version 13.0.0 (clang-1300.0.29.30)", because it's a higher version ? Thanks. -Han Xin
On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote: > On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing > > braces around initialization of a subobject, which is problematic when > > building with `DEVELOPER=YesPlease` which enables `-Werror`: > > > > builtin/unpack-objects.c:388:26: error: suggest braces around > > initialization of subobject [-Werror,-Wmissing-braces] > > git_zstream zstream = { 0 }; > > > > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)" > > - git_zstream zstream = { 0 }; > > + git_zstream zstream = {{ 0 }}; > > Not a comment, just wondering, when should I use "{ { 0 } }" and when > should I use "{ 0 }"? > > I didn't get the error with "Apple clang version 13.0.0 > (clang-1300.0.29.30)", because it's > a higher version ? I don't have a good answer. More modern `clang` versions don't seem to complain about plain old `{0}` here, but the older `clang` with which I'm stuck does complain. Aside from actually building the project with an older `clang` (or older Apple-specific `clang`), it may be sufficient to inspect the structure that's being initialized to see if the first element is itself a subobject. However, I'm not sure it's worth the effort to do so considering how rare this problem seems to be.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote: >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote: >> > On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing >> > braces around initialization of a subobject, which is problematic when >> > building with `DEVELOPER=YesPlease` which enables `-Werror`: >> > >> > builtin/unpack-objects.c:388:26: error: suggest braces around >> > initialization of subobject [-Werror,-Wmissing-braces] >> > git_zstream zstream = { 0 }; >> > >> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)" >> > - git_zstream zstream = { 0 }; >> > + git_zstream zstream = {{ 0 }}; >> >> Not a comment, just wondering, when should I use "{ { 0 } }" and when >> should I use "{ 0 }"? >> >> I didn't get the error with "Apple clang version 13.0.0 >> (clang-1300.0.29.30)", because it's >> a higher version ? > > I don't have a good answer. More modern `clang` versions don't seem to > complain about plain old `{0}` here, but the older `clang` with which > I'm stuck does complain. I think, from the language-lawyer perspective, "{ 0 }" is how we should spell these initialization when we are not using designated initializers, even when the first member of the struct happens to be a struct. The older clang that complains at you is simply buggy, and I think we had the same issue with older sparse.
On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote: > >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > >> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)" > >> > - git_zstream zstream = { 0 }; > >> > + git_zstream zstream = {{ 0 }}; > >> > >> Not a comment, just wondering, when should I use "{ { 0 } }" and when > >> should I use "{ 0 }"? > > > > I don't have a good answer. More modern `clang` versions don't seem to > > complain about plain old `{0}` here, but the older `clang` with which > > I'm stuck does complain. > > I think, from the language-lawyer perspective, "{ 0 }" is how we > should spell these initialization when we are not using designated > initializers, even when the first member of the struct happens to be > a struct. > > The older clang that complains at you is simply buggy, and I think > we had the same issue with older sparse. I can't tell from your response whether or not you intend to pick up this patch. I don't disagree that older clang may be considered buggy in this regard, but older clang versions still exist in the wild, and we already support them by applying `{{0}}` when appropriate: % git grep -n '{ *{ *0 *} *}' builtin/merge-file.c:31: xmparam_t xmp = {{0}}; builtin/worktree.c:262: struct config_set cs = { { 0 } }; oidset.h:25:#define OIDSET_INIT { { 0 } } worktree.c:840: struct config_set cs = { { 0 } }; so the change made by this patch is in line with existing practice on this project.
On Tue, Jul 12 2022, Eric Sunshine wrote: > On Mon, Jul 11, 2022 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> > On Sun, Jul 10, 2022 at 10:00 PM Han Xin <chiyutianyi@gmail.com> wrote: >> >> On Sun, Jul 10, 2022 at 4:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote: >> >> > [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)" >> >> > - git_zstream zstream = { 0 }; >> >> > + git_zstream zstream = {{ 0 }}; >> >> >> >> Not a comment, just wondering, when should I use "{ { 0 } }" and when >> >> should I use "{ 0 }"? >> > >> > I don't have a good answer. More modern `clang` versions don't seem to >> > complain about plain old `{0}` here, but the older `clang` with which >> > I'm stuck does complain. >> >> I think, from the language-lawyer perspective, "{ 0 }" is how we >> should spell these initialization when we are not using designated >> initializers, even when the first member of the struct happens to be >> a struct. >> >> The older clang that complains at you is simply buggy, and I think >> we had the same issue with older sparse. > > I can't tell from your response whether or not you intend to pick up > this patch. I don't disagree that older clang may be considered buggy > in this regard, but older clang versions still exist in the wild, and > we already support them by applying `{{0}}` when appropriate: > > % git grep -n '{ *{ *0 *} *}' > builtin/merge-file.c:31: xmparam_t xmp = {{0}}; Not so fast :) If you check out "next", does compiling builtin/merge-file.o there complain on that clang version now? I changed this to the "{ 0 }" form. If not I wonder if this has to do with one of git_zstream being typedef'd, or with the first member being an embedded struct (I couldn't find another example of that). For the former does the patch at the end & "make builtin/unpack-objects.o" make it go away? > builtin/worktree.c:262: struct config_set cs = { { 0 } }; > oidset.h:25:#define OIDSET_INIT { { 0 } } > worktree.c:840: struct config_set cs = { { 0 } }; Uh, and here are some other examples, so those also warn if you make them just a "{ 0 }"? > so the change made by this patch is in line with existing practice on > this project. It is nice though to be able to use standard C99 consistently, where a "{ 0 }" recursively initializes the members, I think that's what your clang version is doing, it's just complaining about it. Since this is only a warning, and only a practical issue with -Werror I wonder if a config.mak.dev change wouldn't be better, i.e. to provide a -Wno-missing-braces for this older clang version. The ad-hoc test patch referred to above: diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 43789b8ef29..f08092cb26d 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -110,7 +110,7 @@ static void use(int bytes) */ static void *get_data(unsigned long size) { - git_zstream stream; + struct git_zstream stream; unsigned long bufsize = dry_run && size > 8192 ? 8192 : size; void *buf = xmallocz(bufsize); @@ -352,7 +352,7 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size, } struct input_zstream_data { - git_zstream *zstream; + struct git_zstream *zstream; unsigned char buf[8192]; int status; }; @@ -361,7 +361,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream, unsigned long *readlen) { struct input_zstream_data *data = in_stream->data; - git_zstream *zstream = data->zstream; + struct git_zstream *zstream = data->zstream; void *in = fill(1); if (in_stream->is_finished) { @@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream, static void stream_blob(unsigned long size, unsigned nr) { - git_zstream zstream = { 0 }; + struct git_zstream zstream = { 0 }; struct input_zstream_data data = { 0 }; struct input_stream in_stream = { .read = feed_input_zstream, diff --git a/cache.h b/cache.h index ac5ab4ef9d3..797f8e4edae 100644 --- a/cache.h +++ b/cache.h @@ -18,7 +18,7 @@ #include "repository.h" #include "mem-pool.h" -typedef struct git_zstream { +struct git_zstream { z_stream z; unsigned long avail_in; unsigned long avail_out; @@ -26,21 +26,21 @@ typedef struct git_zstream { unsigned long total_out; unsigned char *next_in; unsigned char *next_out; -} git_zstream; - -void git_inflate_init(git_zstream *); -void git_inflate_init_gzip_only(git_zstream *); -void git_inflate_end(git_zstream *); -int git_inflate(git_zstream *, int flush); - -void git_deflate_init(git_zstream *, int level); -void git_deflate_init_gzip(git_zstream *, int level); -void git_deflate_init_raw(git_zstream *, int level); -void git_deflate_end(git_zstream *); -int git_deflate_abort(git_zstream *); -int git_deflate_end_gently(git_zstream *); -int git_deflate(git_zstream *, int flush); -unsigned long git_deflate_bound(git_zstream *, unsigned long); +}; + +void git_inflate_init(struct git_zstream *); +void git_inflate_init_gzip_only(struct git_zstream *); +void git_inflate_end(struct git_zstream *); +int git_inflate(struct git_zstream *, int flush); + +void git_deflate_init(struct git_zstream *, int level); +void git_deflate_init_gzip(struct git_zstream *, int level); +void git_deflate_init_raw(struct git_zstream *, int level); +void git_deflate_end(struct git_zstream *); +int git_deflate_abort(struct git_zstream *); +int git_deflate_end_gently(struct git_zstream *); +int git_deflate(struct git_zstream *, int flush); +unsigned long git_deflate_bound(struct git_zstream *, unsigned long); #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT) #define DTYPE(de) ((de)->d_type) @@ -1372,7 +1372,7 @@ enum unpack_loose_header_result { ULHR_BAD, ULHR_TOO_LONG, }; -enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, +enum unpack_loose_header_result unpack_loose_header(struct git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer,
On Tue, Jul 12, 2022 at 2:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Tue, Jul 12 2022, Eric Sunshine wrote: > > I can't tell from your response whether or not you intend to pick up > > this patch. I don't disagree that older clang may be considered buggy > > in this regard, but older clang versions still exist in the wild, and > > we already support them by applying `{{0}}` when appropriate: > > > > % git grep -n '{ *{ *0 *} *}' > > builtin/merge-file.c:31: xmparam_t xmp = {{0}}; > > Not so fast :) If you check out "next", does compiling > builtin/merge-file.o there complain on that clang version now? I changed > this to the "{ 0 }" form. No, builtin/merge-file.c doesn't compile, and I discovered that just after sending the email to which you responded. I haven't yet prepared a patch for that new instance since I don't know if Junio feels inclined to pick up such a change. > If not I wonder if this has to do with one of git_zstream being > typedef'd, or with the first member being an embedded struct (I couldn't > find another example of that). For the former does the patch at the end > & "make builtin/unpack-objects.o" make it go away? No, the patch you included doesn't make the problem go away (even after I fixed all the "error: must use 'struct' tag to refer to type 'git_zstream'" errors which showed up throughout the project after applying your patch). > > builtin/worktree.c:262: struct config_set cs = { { 0 } }; > > oidset.h:25:#define OIDSET_INIT { { 0 } } > > worktree.c:840: struct config_set cs = { { 0 } }; > > Uh, and here are some other examples, so those also warn if you make > them just a "{ 0 }"? Yes. (Full disclosure: Even though the two worktree-related instances come from commits by Stolee, I'm pretty sure I'm the one who asked him to change them from `{0}` to `{{0}}` during review for this very reason.) > > so the change made by this patch is in line with existing practice on > > this project. > > It is nice though to be able to use standard C99 consistently, where a > "{ 0 }" recursively initializes the members, I think that's what your > clang version is doing, it's just complaining about it. Agreed, it would be nice to use plain `{0}`. > Since this is only a warning, and only a practical issue with -Werror I > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a > -Wno-missing-braces for this older clang version. I'm in favor of this. It would, of course, require extra special-casing for Apple's clang for which the version number bears no resemblance to reality since Apple invents their own version numbers.
On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote: > > Since this is only a warning, and only a practical issue with -Werror I > > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a > > -Wno-missing-braces for this older clang version. > > I'm in favor of this. It would, of course, require extra > special-casing for Apple's clang for which the version number bears no > resemblance to reality since Apple invents their own version numbers. I got PTSD reading that thread again, but in case anybody wants to dig into this, I think there are some hints from the last time we discussed this (starting at the end of this message and the subthread): https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/ -Peff
On Tue, Jul 12, 2022 at 3:23 AM Jeff King <peff@peff.net> wrote: > On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote: > > > Since this is only a warning, and only a practical issue with -Werror I > > > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a > > > -Wno-missing-braces for this older clang version. > > > > I'm in favor of this. It would, of course, require extra > > special-casing for Apple's clang for which the version number bears no > > resemblance to reality since Apple invents their own version numbers. > > I got PTSD reading that thread again, but in case anybody wants to dig > into this, I think there are some hints from the last time we discussed > this (starting at the end of this message and the subthread): > > https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/ Thanks for digging up that link, and sorry for triggering PTSD.
On Tue, Jul 12 2022, Jeff King wrote: > On Tue, Jul 12, 2022 at 03:13:50AM -0400, Eric Sunshine wrote: > >> > Since this is only a warning, and only a practical issue with -Werror I >> > wonder if a config.mak.dev change wouldn't be better, i.e. to provide a >> > -Wno-missing-braces for this older clang version. >> >> I'm in favor of this. It would, of course, require extra >> special-casing for Apple's clang for which the version number bears no >> resemblance to reality since Apple invents their own version numbers. FWIW I was imagining just providing that -Wno-* on clang versions <= 11, not special-casing Apple's in particular. If you want to make it more strict you can always compare against the uname, at this point in config.mak.dev we've already sourced config.mak.uname, so you can guard this with "ifeq ($(uname_S),Darwin)". Of course that doesn't tell you if it's Apple's clang, just "a clang on Apple", but it should be close enough not to matter... > I got PTSD reading that thread again, but in case anybody wants to dig > into this, I think there are some hints from the last time we discussed > this (starting at the end of this message and the subthread): > > https://lore.kernel.org/git/YQ2LdvwEnZN9LUQn@coredump.intra.peff.net/ Oh yes, the config.mak.dev horror show :) I have a local patches that carry forward the idea I had in that thread, i.e. to drop all this version detection insanity and just compile a C program to detect the compiler. It takes a bit of doing in the Makefile, but I think the end result is lovely compared to the status quo. We just do: $ head -n 2 config.mak.dev include .build/probe/compiler.mak include .build/probe/config-mak-dev.mak [The rest is all using existing defined variables, no shell magic] Which is just made with a Makefile by piping this sort of thing to those .build files: $ ./.build/probe/config-mak-dev PROBE_COMPILER_NEEDS_std-eq-gnu99 = 1 PROBE_COMPILER_HAS_Wtautological-constant-out-of-range-compare = 1 PROBE_COMPILER_HAS_Wextra = 1 PROBE_COMPILER_HAS_Wpedantic = 1 Which in turn is generated with stand-alone C programs in probe/, which don't need any of the rest of git: $ cat probe/config-mak-dev.c #ifdef PROBE_STANDALONE #include <stdlib.h> #else #include "git-compat-util.h" #endif #include "probe/compiler.h" #ifdef __GLIBC__ #include <gnu/libc-version.h> #endif int probe_config_mak_dev(probe_info_fn_t fn, void *util) { #ifdef __clang__ #if __clang_major__ >= 7 fn(util, "NEEDS_std-eq-gnu99", "1"); #endif #ifndef __has_warning #error "Clang version too old to support __has_warning!" #endif #if __has_warning("-Wtautological-constant-out-of-range-compare") fn(util, "HAS_Wtautological-constant-out-of-range-compare", "1"); #endif #if __has_warning("-Wextra") fn(util, "HAS_Wextra", "1"); #endif #if __has_warning("-Wpedantic") fn(util, "HAS_Wpedantic", "1"); #endif /* __clang__ */ #elif defined(__GNUC__) #if __GNUC__ == 4 fn(util, "NEEDS_Wno-uninitialized", "1"); #endif #if __GNUC__ >= 5 fn(util, "HAS_Wpedantic", "1"); #if __GNUC__ >= 6 fn(util, "NEEDS_std-eq-gnu99", "1"); fn(util, "HAS_Wextra", "1"); #if __GNUC__ >= 10 fn(util, "HAS_Wno-pedantic-ms-format", "1"); #endif /* >= 10 */ #endif /* >= 6 */ #endif /* >= 5 */ #elif defined(__IBMC__) #else return -1; #endif return 0; } #ifdef PROBE_STANDALONE #include <stdio.h> #include "probe/print.h" int main(void) { struct probe_print_data data = { .prefix = "PROBE_COMPILER_", }; if (probe_config_mak_dev(probe_print, &data) < 0) fprintf(stderr, "warning: unable to detect compiler type and version\n"); return 0; } #endif The compilation is then triggered by the include in config.mak.dev, which has a corresponding rule that creates the C program, then the generated *.mak, so once we do it once we're only ever including an already generated text file. It takes a bit of doing in the Makfile, since we need to e.g. declare that "artifacts-tar", "check-docs" etc. don't want to build this C program to "configure bootstrap" even if under DEVELOPER=1, i.e. we need to know which target(s) we'll run to compile C code. But that has the bonus benefit of making those faster, as now we'll e.g. $(shell detect-compiler), generate the version info etc., only to run "$(MAKE) -C Documentation/ ..." or whatever. I can clean it up for submission if there's interest.
Eric Sunshine <sunshine@sunshineco.com> writes: >> > % git grep -n '{ *{ *0 *} *}' >> > builtin/merge-file.c:31: xmparam_t xmp = {{0}}; >> >> Not so fast :) If you check out "next", does compiling >> builtin/merge-file.o there complain on that clang version now? I changed >> this to the "{ 0 }" form. > > No, builtin/merge-file.c doesn't compile, and I discovered that just > after sending the email to which you responded. I haven't yet prepared > a patch for that new instance since I don't know if Junio feels > inclined to pick up such a change. Wait, what do you mean by "doesn't compile"? The compiler totally chokes on "{ 0 } recursively zero initializes" idiom and does not know what binary to produce, or it merely warns even though it knows what to do with the code, but because we choose to give -Werror, it is stopped from producing a binary? >> It is nice though to be able to use standard C99 consistently, where a >> "{ 0 }" recursively initializes the members, I think that's what your >> clang version is doing, it's just complaining about it. > > Agreed, it would be nice to use plain `{0}`. > >> Since this is only a warning, and only a practical issue with -Werror I >> wonder if a config.mak.dev change wouldn't be better, i.e. to provide a >> -Wno-missing-braces for this older clang version. > > I'm in favor of this. It would, of course, require extra > special-casing for Apple's clang for which the version number bears no > resemblance to reality since Apple invents their own version numbers. I guess from this that you meant "we get an erroneous warning". If so, I am in favor of squelching the warning.
On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> I'm in favor of this. It would, of course, require extra > >> special-casing for Apple's clang for which the version number bears no > >> resemblance to reality since Apple invents their own version numbers. > > FWIW I was imagining just providing that -Wno-* on clang versions <= 11, > not special-casing Apple's in particular. It's not about special-casing Apple in particular. It's that our detect-compiler script does not understand which version of clang is used for Apple's compiler. Their version numbers are totally mismatched. So you either have to say "turn off this warning for clang totally", or do the wrong thing when Apple's compiler is in use. > I have a local patches that carry forward the idea I had in that thread, > i.e. to drop all this version detection insanity and just compile a C > program to detect the compiler. Hmm. I prefer your earlier suggestion to use "$(CC) -E". This tool has to run on every invocation of "make", so the lighter-weight it is, the better. You say later... > The compilation is then triggered by the include in config.mak.dev, > which has a corresponding rule that creates the C program, then the > generated *.mak, so once we do it once we're only ever including an > already generated text file. but that implies we don't have a dependency on the compiler itself. You'd want to at least depend on the name of the compiler. But that would also miss if running "clang" changes which version of clang you're running. I know upgrading your compiler is rare-ish, but this is exactly the kind of thing I expect to bite at the most annoying time (when you're switching around versions to try to figure out how they behave). > #ifdef __clang__ > #if __clang_major__ >= 7 > fn(util, "NEEDS_std-eq-gnu99", "1"); > #endif This is still gross version detection, but I don't think we can avoid it. However... > #if __has_warning("-Wextra") > fn(util, "HAS_Wextra", "1"); > #endif ...this is much nicer. It could still be implemented purely via "-E", as far as I can see, like: #if __has_warning("-Wextra") HAS_Wextra = 1 #endif But then we end up having to do version comparisons for gcc anyway: > #if __GNUC__ >= 6 > fn(util, "NEEDS_std-eq-gnu99", "1"); > fn(util, "HAS_Wextra", "1"); so it feels like we're back where we started. You've just encoded the version checks in a different spot. I dunno. I don't find this significantly less gross than the status quo. I don't mind getting the version via "-E" rather than "-v", but whether the policy logic is in cpp, or in shell, or in the Makefile, it still needs to exist. Putting it in cpp allows using has_warning(), but since that isn't available everywhere, I'm not sure it buys us much. -Peff
On Thu, Jul 14 2022, Jeff King wrote: > On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> >> I'm in favor of this. It would, of course, require extra >> >> special-casing for Apple's clang for which the version number bears no >> >> resemblance to reality since Apple invents their own version numbers. >> >> FWIW I was imagining just providing that -Wno-* on clang versions <= 11, >> not special-casing Apple's in particular. > > It's not about special-casing Apple in particular. It's that our > detect-compiler script does not understand which version of clang is > used for Apple's compiler. Their version numbers are totally mismatched. > > So you either have to say "turn off this warning for clang totally", or > do the wrong thing when Apple's compiler is in use. I missed the part where we don't even detect the version in that case, but anyway, just turning it off on clang && OSX seems fine. This is -Wmissing-braces, we'll catch it elsewhere (CI etc.) >> I have a local patches that carry forward the idea I had in that thread, >> i.e. to drop all this version detection insanity and just compile a C >> program to detect the compiler. > > Hmm. I prefer your earlier suggestion to use "$(CC) -E". This tool has > to run on every invocation of "make", so the lighter-weight it is, the > better. You say later... We only compile & run the binary once, then save the result to a *.mak file, the intention is to get rid of running these binaries or script on every single invocation. >> The compilation is then triggered by the include in config.mak.dev, >> which has a corresponding rule that creates the C program, then the >> generated *.mak, so once we do it once we're only ever including an >> already generated text file. > > but that implies we don't have a dependency on the compiler itself. > You'd want to at least depend on the name of the compiler. But that > would also miss if running "clang" changes which version of clang you're > running. I know upgrading your compiler is rare-ish, but this is exactly > the kind of thing I expect to bite at the most annoying time (when > you're switching around versions to try to figure out how they behave). That's true, I considered that OK, and still do. I.e. we use this for config.mak, where we are opting you in to never flags. If your compiler name changes your GIT-BUILD-OPTIONS will change, so we'll re-build and re-detect. But yes, if your CC=gcc and you change gcc from under us we won't re-detect and re-build. But isn't that fine? You just won't get newer flags, you might downgrade your gcc and get an error, but generally speaking make-based systems really don't try to detect "anything on the OS" changed. Still, I could easily add something where we one-off run "command -v $(CC)", save that to a file, and then add that to our "make" dependency tree. So then if the compiler binary's mtime changes we'd re-build, and we still wouldn't run something on every invocation. It just seems to be quite pedantic about correctness, is all :) >> #ifdef __clang__ >> #if __clang_major__ >= 7 >> fn(util, "NEEDS_std-eq-gnu99", "1"); >> #endif > > This is still gross version detection, but I don't think we can avoid > it. However... Yes, this part isn't really nicer, although with this method we could also avoid this sort of thing by just trying the flag out & caching that, but this seemed OK. >> #if __has_warning("-Wextra") >> fn(util, "HAS_Wextra", "1"); >> #endif > > ...this is much nicer. It could still be implemented purely via "-E", as > far as I can see, like: > > #if __has_warning("-Wextra") > HAS_Wextra = 1 > #endif *nod*, I wish GCC had that. > But then we end up having to do version comparisons for gcc anyway: > >> #if __GNUC__ >= 6 >> fn(util, "NEEDS_std-eq-gnu99", "1"); >> fn(util, "HAS_Wextra", "1"); > > so it feels like we're back where we started. You've just encoded the > version checks in a different spot. We're really not, the reason I did this was because i tried to add support for xlc and suncc to this script, we don't even parse detect-compiler on Solaris now, as its /bin/sh isn't compatible with it. So to begin with we'd need to hoist the "shell detection and script generation" out ... > I dunno. I don't find this significantly less gross than the status quo. > I don't mind getting the version via "-E" rather than "-v", but whether > the policy logic is in cpp, or in shell, or in the Makefile, it still > needs to exist. Putting it in cpp allows using has_warning(), but since > that isn't available everywhere, I'm not sure it buys us much. Compilers universally support "who and what am I?" via their native macros, but we're trying to do it the really hard way via parsing version output.
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 43789b8ef2..c606c92e37 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -385,7 +385,7 @@ static const void *feed_input_zstream(struct input_stream *in_stream, static void stream_blob(unsigned long size, unsigned nr) { - git_zstream zstream = { 0 }; + git_zstream zstream = {{ 0 }}; struct input_zstream_data data = { 0 }; struct input_stream in_stream = { .read = feed_input_zstream,
On macOS High Sierra (10.13), Apple's `clang`[1] complains about missing braces around initialization of a subobject, which is problematic when building with `DEVELOPER=YesPlease` which enables `-Werror`: builtin/unpack-objects.c:388:26: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] git_zstream zstream = { 0 }; [1]: `cc --version` => "Apple LLVM version 10.0.0 (clang-1000.10.44.4)" Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- Notes: This is atop 'hx/unpack-streaming' which is already in 'next'. All the CI builds are fine with this change. As I understand it, this should be a safe change; the fields which follow `z_stream z` in `git_zstream` will be initialized to zero since the first field has an explicit initializer. builtin/unpack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)