| Message ID | CAESRpQDO8NVsXr0oHJQe5LEgP67iyx6nTXspiwrKyn9Vp4cN1g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 12 November 2014 15:54, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > On 12 November 2014 15:38, Marek Polacek <polacek@redhat.com> wrote: >> On Wed, Nov 12, 2014 at 03:35:06PM +0100, Manuel López-Ibáñez wrote: >>> > ../../libcpp/line-map.c:667:65: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] >>> >>> I just (r217418) bootstrapped this code and it did not produce this >>> error (or warning). Could you give more details? >> >> Have you tried the bootstrap without checking enabled? > > Indeed, the error is due to linemap_assert definition. My patch just > exposes the bug. This should fix it: And I think GCC is wrong to wan here. The point of the Wempty-body warning is to catch things like: if(a); return 2; return 3; However, if(a) ; return 2; seems intentional. Clang++ does not warn on the latter and it prints for the former: warning: if statement has empty body [-Wempty-body] if(a); ^ note: put the semicolon on a separate line to silence this warning which seems a nice way to silence the warning instead of ugly { ; } Cheers, Manuel.
On Nov 12, 2014, at 7:04 AM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > And I think GCC is wrong to wan here. The point of the Wempty-body > warning is to catch things like: > > if(a); > return 2; > return 3; > > However, > > if(a) > ; > return 2; In the olden days, we didn’t have enough information to do the warnings well. clang did better, cause it always had the information necessary. I think if (); should warn, and if () ; should not, neither should if () \n ;, if we have the information.
On Wed, Nov 12, 2014 at 01:24:18PM -0800, Mike Stump wrote: > On Nov 12, 2014, at 7:04 AM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > > And I think GCC is wrong to wan here. The point of the Wempty-body > > warning is to catch things like: > > > > if(a); > > return 2; > > return 3; > > > > However, > > > > if(a) > > ; > > return 2; > > In the olden days, we didn’t have enough information to do the warnings > well. clang did better, cause it always had the information necessary. I > think if (); should warn, and if () ; should not, neither should if () \n > ;, if we have the information. I think we had discussions on this topic, the important thing is that we don't want to generate different warnings based on whether -save-temps has been used, there could be just comment in between ) and ; etc. Jakub
On 12 November 2014 22:41, Jakub Jelinek <jakub@redhat.com> wrote: > I think we had discussions on this topic, the important thing is that we > don't want to generate different warnings based on whether -save-temps has > been used, there could be just comment in between ) and ; etc. How can --save-temps influence whether ";" is in the same line as the 'if' or not? Cheers, Manuel.
On Nov 12, 2014, at 3:52 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > On 12 November 2014 22:41, Jakub Jelinek <jakub@redhat.com> wrote: >> I think we had discussions on this topic, the important thing is that we >> don't want to generate different warnings based on whether -save-temps has >> been used, there could be just comment in between ) and ; etc. > > How can --save-temps influence whether ";" is in the same line as the > 'if' or not? He was worried about: int main() { if (1)/*hi*/; } mrs@alcmene:~/work1/gcc-c2e/gcc$ gcc -E tt.c int main() { if (1) ; } but, no need to worry, there is a space in there. People that get this wrong do );, without a space. They also don’t have a comment in there either. The eye disappears the ; when right next to ), but, if there is a space, the eye can find it pretty easy.
On 13 November 2014 03:07, Mike Stump <mikestump@comcast.net> wrote: > On Nov 12, 2014, at 3:52 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: >> On 12 November 2014 22:41, Jakub Jelinek <jakub@redhat.com> wrote: >>> I think we had discussions on this topic, the important thing is that we >>> don't want to generate different warnings based on whether -save-temps has >>> been used, there could be just comment in between ) and ; etc. >> >> How can --save-temps influence whether ";" is in the same line as the >> 'if' or not? > > He was worried about: > > int main() { > if (1)/*hi*/; > } > mrs@alcmene:~/work1/gcc-c2e/gcc$ gcc -E tt.c > int main() { > if (1) ; > } > > but, no need to worry, there is a space in there. People that get this wrong do );, without a space. They also don't have a comment in there either. The eye disappears the ; when right next to ), but, if there is a space, the eye can find it pretty easy. But that has nothing to do with the change I proposed, which is to not warn if ";" is on a different line. The actual heuristics of Clang are more complicated and avoid even more false positives, but our location info is not precise enough yet to implement them. Cheers, Manuel.
On 12 November 2014 15:54, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > On 12 November 2014 15:38, Marek Polacek <polacek@redhat.com> wrote: >> On Wed, Nov 12, 2014 at 03:35:06PM +0100, Manuel López-Ibáñez wrote: >>> > ../../libcpp/line-map.c:667:65: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] >>> >>> I just (r217418) bootstrapped this code and it did not produce this >>> error (or warning). Could you give more details? >> >> Have you tried the bootstrap without checking enabled? > > Indeed, the error is due to linemap_assert definition. My patch just > exposes the bug. This should fix it: > > Index: line-map.h > =================================================================== > --- line-map.h (revision 217418) > +++ line-map.h (working copy) > @@ -584,11 +584,12 @@ bool linemap_location_from_macro_expansi > the replacement-list of a macro expansion. */ > #define linemap_check_ordinary(LINE_MAP) __extension__ \ > ({linemap_assert (!linemap_macro_expansion_map_p (LINE_MAP)); \ > (LINE_MAP);}) > #else > -#define linemap_assert(EXPR) > +/* Include EXPR, so that unused variable warnings do not occur. */ > +#define linemap_assert(EXPR) ((void)(0 && (EXPR))) > #define linemap_check_ordinary(LINE_MAP) (LINE_MAP) > #endif > > /* Encode and return a source_location from a column number. The > source line considered is the last source line used to call > > (It really sucks that libcpp and by extension line-map cannot use gcc > code: gcc_checking_assert was already correct. What a boring > duplicated effort!) > > I can commit the above as obvious tonite (if no one else takes care of > fixing it before me). It seems marxin committed this as revision 217473, thus this issue should be fixed now. Cheers, Manuel.
Index: line-map.h =================================================================== --- line-map.h (revision 217418) +++ line-map.h (working copy) @@ -584,11 +584,12 @@ bool linemap_location_from_macro_expansi the replacement-list of a macro expansion. */ #define linemap_check_ordinary(LINE_MAP) __extension__ \ ({linemap_assert (!linemap_macro_expansion_map_p (LINE_MAP)); \ (LINE_MAP);}) #else -#define linemap_assert(EXPR) +/* Include EXPR, so that unused variable warnings do not occur. */ +#define linemap_assert(EXPR) ((void)(0 && (EXPR))) #define linemap_check_ordinary(LINE_MAP) (LINE_MAP) #endif /* Encode and return a source_location from a column number. The source line considered is the last source line used to call