[PR,c/52952] More precise locations within format strings
diff mbox

Message ID CAESRpQDO8NVsXr0oHJQe5LEgP67iyx6nTXspiwrKyn9Vp4cN1g@mail.gmail.com
State New
Headers show

Commit Message

Manuel López-Ibáñez Nov. 12, 2014, 2:54 p.m. UTC
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:


(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).

Cheers,

Manuel.

Comments

Manuel López-Ibáñez Nov. 12, 2014, 3:04 p.m. UTC | #1
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.
Mike Stump Nov. 12, 2014, 9:24 p.m. UTC | #2
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.
Jakub Jelinek Nov. 12, 2014, 9:41 p.m. UTC | #3
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
Manuel López-Ibáñez Nov. 12, 2014, 11:52 p.m. UTC | #4
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.
Mike Stump Nov. 13, 2014, 2:07 a.m. UTC | #5
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.
Manuel López-Ibáñez Nov. 13, 2014, 2:22 a.m. UTC | #6
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.
Manuel López-Ibáñez Nov. 13, 2014, 4:41 p.m. UTC | #7
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.

Patch
diff mbox

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