| Message ID | 1498537095-627-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.06.2017 06:18, Thomas Huth wrote: > The intercept test currently can not be compiled with GCC 4.8 anymore. > It generates the following warning (which is fatal due to -Werror): > > s390x/intercept.c: In function ‘test_stidp’: > s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] > struct cpuid id = {}; > ^ > Fix it by using a "0" as intializer here. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > NB: We could also remove the -Wextra from the CFLAGS instead. IMHO > using -Wextra together with -Werror is just like playing Russian roulette. > Since -Wextra is some kind of "compiler warning playground" for the GCC > folks, you never know which compiler version will trigger an unexpected > (and often also unfounded) warning here, so using this together with -Werror > is just a nuisance. I agree, this is really not deterministic. > > s390x/intercept.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/s390x/intercept.c b/s390x/intercept.c > index 9766289..9fe86cf 100644 > --- a/s390x/intercept.c > +++ b/s390x/intercept.c > @@ -108,7 +108,7 @@ static void test_stap(void) > /* Test the STORE CPU ID instruction */ > static void test_stidp(void) > { > - struct cpuid id = {}; > + struct cpuid id = { 0 }; > > asm volatile ("stidp %0\n" : "+Q"(id)); > report("type set", id.type); > arm and powerpc also use -Wextra, maybe we should remove this then for all. Whatever you prefer. Reviewed-by: David Hildenbrand <david@redhat.com>
On 27.06.2017 10:24, David Hildenbrand wrote: > On 27.06.2017 06:18, Thomas Huth wrote: >> The intercept test currently can not be compiled with GCC 4.8 anymore. >> It generates the following warning (which is fatal due to -Werror): >> >> s390x/intercept.c: In function ‘test_stidp’: >> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] >> struct cpuid id = {}; >> ^ >> Fix it by using a "0" as intializer here. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> NB: We could also remove the -Wextra from the CFLAGS instead. IMHO >> using -Wextra together with -Werror is just like playing Russian roulette. >> Since -Wextra is some kind of "compiler warning playground" for the GCC >> folks, you never know which compiler version will trigger an unexpected >> (and often also unfounded) warning here, so using this together with -Werror >> is just a nuisance. > > I agree, this is really not deterministic. > >> >> s390x/intercept.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/s390x/intercept.c b/s390x/intercept.c >> index 9766289..9fe86cf 100644 >> --- a/s390x/intercept.c >> +++ b/s390x/intercept.c >> @@ -108,7 +108,7 @@ static void test_stap(void) >> /* Test the STORE CPU ID instruction */ >> static void test_stidp(void) >> { >> - struct cpuid id = {}; >> + struct cpuid id = { 0 }; >> >> asm volatile ("stidp %0\n" : "+Q"(id)); >> report("type set", id.type); >> > > arm and powerpc also use -Wextra, maybe we should remove this then for all. > > Whatever you prefer. True ... maybe Drew and Laurent can also comment on whether they like -Wextra or not ... if we all agree, then we can remove it, otherwise let's try to go with this patch first (in the hope that we won't hit the next problem too soon). > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks! Thomas
On 27/06/2017 10:33, Thomas Huth wrote: > On 27.06.2017 10:24, David Hildenbrand wrote: >> On 27.06.2017 06:18, Thomas Huth wrote: >>> The intercept test currently can not be compiled with GCC 4.8 anymore. >>> It generates the following warning (which is fatal due to -Werror): >>> >>> s390x/intercept.c: In function ‘test_stidp’: >>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] >>> struct cpuid id = {}; >>> ^ >>> Fix it by using a "0" as intializer here. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> NB: We could also remove the -Wextra from the CFLAGS instead. IMHO >>> using -Wextra together with -Werror is just like playing Russian roulette. >>> Since -Wextra is some kind of "compiler warning playground" for the GCC >>> folks, you never know which compiler version will trigger an unexpected >>> (and often also unfounded) warning here, so using this together with -Werror >>> is just a nuisance. >> >> I agree, this is really not deterministic. >> >>> >>> s390x/intercept.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/s390x/intercept.c b/s390x/intercept.c >>> index 9766289..9fe86cf 100644 >>> --- a/s390x/intercept.c >>> +++ b/s390x/intercept.c >>> @@ -108,7 +108,7 @@ static void test_stap(void) >>> /* Test the STORE CPU ID instruction */ >>> static void test_stidp(void) >>> { >>> - struct cpuid id = {}; >>> + struct cpuid id = { 0 }; >>> >>> asm volatile ("stidp %0\n" : "+Q"(id)); >>> report("type set", id.type); >>> >> >> arm and powerpc also use -Wextra, maybe we should remove this then for all. >> >> Whatever you prefer. > > True ... maybe Drew and Laurent can also comment on whether they like > -Wextra or not ... if we all agree, then we can remove it, otherwise > let's try to go with this patch first (in the hope that we won't hit the > next problem too soon). I like -Wextra :) My opinion is more checking we have, less error we have. The extra cost of adding some initializers is nothing. But we can consider this as details, so do as you want... Thanks, Laurent
On 27.06.2017 10:53, Laurent Vivier wrote: > On 27/06/2017 10:33, Thomas Huth wrote: >> On 27.06.2017 10:24, David Hildenbrand wrote: >>> On 27.06.2017 06:18, Thomas Huth wrote: >>>> The intercept test currently can not be compiled with GCC 4.8 anymore. >>>> It generates the following warning (which is fatal due to -Werror): >>>> >>>> s390x/intercept.c: In function ‘test_stidp’: >>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] >>>> struct cpuid id = {}; >>>> ^ >>>> Fix it by using a "0" as intializer here. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> NB: We could also remove the -Wextra from the CFLAGS instead. IMHO >>>> using -Wextra together with -Werror is just like playing Russian roulette. >>>> Since -Wextra is some kind of "compiler warning playground" for the GCC >>>> folks, you never know which compiler version will trigger an unexpected >>>> (and often also unfounded) warning here, so using this together with -Werror >>>> is just a nuisance. >>> >>> I agree, this is really not deterministic. >>> >>>> >>>> s390x/intercept.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/s390x/intercept.c b/s390x/intercept.c >>>> index 9766289..9fe86cf 100644 >>>> --- a/s390x/intercept.c >>>> +++ b/s390x/intercept.c >>>> @@ -108,7 +108,7 @@ static void test_stap(void) >>>> /* Test the STORE CPU ID instruction */ >>>> static void test_stidp(void) >>>> { >>>> - struct cpuid id = {}; >>>> + struct cpuid id = { 0 }; >>>> >>>> asm volatile ("stidp %0\n" : "+Q"(id)); >>>> report("type set", id.type); >>>> >>> >>> arm and powerpc also use -Wextra, maybe we should remove this then for all. >>> >>> Whatever you prefer. >> >> True ... maybe Drew and Laurent can also comment on whether they like >> -Wextra or not ... if we all agree, then we can remove it, otherwise >> let's try to go with this patch first (in the hope that we won't hit the >> next problem too soon). > > I like -Wextra :) > > My opinion is more checking we have, less error we have. The problem is that -Wextra often produces unfounded or even wrong warnings, like in this case. I'd prefer to add the individual parameters that rather always make sense instead, like -Wtype-limits for example. Thomas
On 27/06/2017 11:03, Thomas Huth wrote: > On 27.06.2017 10:53, Laurent Vivier wrote: >> On 27/06/2017 10:33, Thomas Huth wrote: >>> On 27.06.2017 10:24, David Hildenbrand wrote: >>>> On 27.06.2017 06:18, Thomas Huth wrote: >>>>> The intercept test currently can not be compiled with GCC 4.8 anymore. >>>>> It generates the following warning (which is fatal due to -Werror): >>>>> >>>>> s390x/intercept.c: In function ‘test_stidp’: >>>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] >>>>> struct cpuid id = {}; >>>>> ^ >>>>> Fix it by using a "0" as intializer here. >>>>> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> NB: We could also remove the -Wextra from the CFLAGS instead. IMHO >>>>> using -Wextra together with -Werror is just like playing Russian roulette. >>>>> Since -Wextra is some kind of "compiler warning playground" for the GCC >>>>> folks, you never know which compiler version will trigger an unexpected >>>>> (and often also unfounded) warning here, so using this together with -Werror >>>>> is just a nuisance. >>>> >>>> I agree, this is really not deterministic. >>>> >>>>> >>>>> s390x/intercept.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/s390x/intercept.c b/s390x/intercept.c >>>>> index 9766289..9fe86cf 100644 >>>>> --- a/s390x/intercept.c >>>>> +++ b/s390x/intercept.c >>>>> @@ -108,7 +108,7 @@ static void test_stap(void) >>>>> /* Test the STORE CPU ID instruction */ >>>>> static void test_stidp(void) >>>>> { >>>>> - struct cpuid id = {}; >>>>> + struct cpuid id = { 0 }; >>>>> >>>>> asm volatile ("stidp %0\n" : "+Q"(id)); >>>>> report("type set", id.type); >>>>> >>>> >>>> arm and powerpc also use -Wextra, maybe we should remove this then for all. >>>> >>>> Whatever you prefer. >>> >>> True ... maybe Drew and Laurent can also comment on whether they like >>> -Wextra or not ... if we all agree, then we can remove it, otherwise >>> let's try to go with this patch first (in the hope that we won't hit the >>> next problem too soon). >> >> I like -Wextra :) >> >> My opinion is more checking we have, less error we have. > > The problem is that -Wextra often produces unfounded or even wrong > warnings, like in this case. I'd prefer to add the individual parameters > that rather always make sense instead, like -Wtype-limits for example. I agree, removing -Wextra and adding individual parameters seems to be a good solution. Laurent
On Tue, Jun 27, 2017 at 11:28:54AM +0200, Laurent Vivier wrote: > On 27/06/2017 11:03, Thomas Huth wrote: > > On 27.06.2017 10:53, Laurent Vivier wrote: > >> On 27/06/2017 10:33, Thomas Huth wrote: > >>> On 27.06.2017 10:24, David Hildenbrand wrote: > >>>> On 27.06.2017 06:18, Thomas Huth wrote: > >>>>> The intercept test currently can not be compiled with GCC 4.8 anymore. > >>>>> It generates the following warning (which is fatal due to -Werror): > >>>>> > >>>>> s390x/intercept.c: In function ‘test_stidp’: > >>>>> s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] > >>>>> struct cpuid id = {}; > >>>>> ^ > >>>>> Fix it by using a "0" as intializer here. > >>>>> > >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>>>> --- > >>>>> NB: We could also remove the -Wextra from the CFLAGS instead. IMHO > >>>>> using -Wextra together with -Werror is just like playing Russian roulette. > >>>>> Since -Wextra is some kind of "compiler warning playground" for the GCC > >>>>> folks, you never know which compiler version will trigger an unexpected > >>>>> (and often also unfounded) warning here, so using this together with -Werror > >>>>> is just a nuisance. > >>>> > >>>> I agree, this is really not deterministic. > >>>> > >>>>> > >>>>> s390x/intercept.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/s390x/intercept.c b/s390x/intercept.c > >>>>> index 9766289..9fe86cf 100644 > >>>>> --- a/s390x/intercept.c > >>>>> +++ b/s390x/intercept.c > >>>>> @@ -108,7 +108,7 @@ static void test_stap(void) > >>>>> /* Test the STORE CPU ID instruction */ > >>>>> static void test_stidp(void) > >>>>> { > >>>>> - struct cpuid id = {}; > >>>>> + struct cpuid id = { 0 }; > >>>>> > >>>>> asm volatile ("stidp %0\n" : "+Q"(id)); > >>>>> report("type set", id.type); > >>>>> > >>>> > >>>> arm and powerpc also use -Wextra, maybe we should remove this then for all. > >>>> > >>>> Whatever you prefer. > >>> > >>> True ... maybe Drew and Laurent can also comment on whether they like > >>> -Wextra or not ... if we all agree, then we can remove it, otherwise > >>> let's try to go with this patch first (in the hope that we won't hit the > >>> next problem too soon). > >> > >> I like -Wextra :) > >> > >> My opinion is more checking we have, less error we have. > > > > The problem is that -Wextra often produces unfounded or even wrong > > warnings, like in this case. I'd prefer to add the individual parameters > > that rather always make sense instead, like -Wtype-limits for example. > > I agree, removing -Wextra and adding individual parameters seems to be a > good solution. Fine by me. I'll happily review whatever somebody posts :-) Thanks, drew
On 27/06/2017 13:32, Andrew Jones wrote: >>> The problem is that -Wextra often produces unfounded or even wrong >>> warnings, like in this case. I'd prefer to add the individual parameters >>> that rather always make sense instead, like -Wtype-limits for example. >> I agree, removing -Wextra and adding individual parameters seems to be a >> good solution. > Fine by me. I'll happily review whatever somebody posts :-) And I'll happily apply a patch that just removes -Wextra in the meanwhile. :) Paolo
diff --git a/s390x/intercept.c b/s390x/intercept.c index 9766289..9fe86cf 100644 --- a/s390x/intercept.c +++ b/s390x/intercept.c @@ -108,7 +108,7 @@ static void test_stap(void) /* Test the STORE CPU ID instruction */ static void test_stidp(void) { - struct cpuid id = {}; + struct cpuid id = { 0 }; asm volatile ("stidp %0\n" : "+Q"(id)); report("type set", id.type);
The intercept test currently can not be compiled with GCC 4.8 anymore. It generates the following warning (which is fatal due to -Werror): s390x/intercept.c: In function ‘test_stidp’: s390x/intercept.c:111:9: error: missing initializer for field ‘version’ of ‘struct cpuid’ [-Werror=missing-field-initializers] struct cpuid id = {}; ^ Fix it by using a "0" as intializer here. Signed-off-by: Thomas Huth <thuth@redhat.com> --- NB: We could also remove the -Wextra from the CFLAGS instead. IMHO using -Wextra together with -Werror is just like playing Russian roulette. Since -Wextra is some kind of "compiler warning playground" for the GCC folks, you never know which compiler version will trigger an unexpected (and often also unfounded) warning here, so using this together with -Werror is just a nuisance. s390x/intercept.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)