Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek <polacek@redhat.com>]

On Thu, Jun 08, 2017 at 12:01:03PM +0100, Pedro Alves wrote:
> On 06/08/2017 11:29 AM, Marek Polacek wrote:
> > On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote:
> >> Hi Marek,
> >>
> >> Nice warning!  Just to confirm, would the patch warn with code like:
> >  
> > Thanks.  BTW, if you (or anyone) could come up with a better name,
> > I'm all ears.
> 
> AFAICS, the warning's intent is catching the case of a
> a macro expanding to multiple (top level) statements, not lines.

True.  I felt that it was implicitly understood what's meant by that,
but I'll change that.  Martin pointed this out, too.

> Both the comments in the code and the description of the
> warning talk in those terms:
> 
>  +/* (....) This warning warns about
>  +   cases when a macro expands to multiple statements not wrapped in
>  +   do {} while (0) or ({ }) and is used as a body of if/else/for/while
>  +   conditionals.  For example,
> 
>  +Wmultiline-expansion
>  +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
>  +Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for.
> 
> So it'd seem clearer to me if the warning was named around
> "-Wmulti-statement-something" instead of "-Wmultiline-something"?
> 
>   -Wmulti-statement-expansion
>   -Wmulti-statement-macros     
>   -Wmulti-statement-macro     
>   -Wmulti-statement-macro-expansion

I think I'll go with -Wmultistatement-expansion (without the dash).

> Particularly when one could argue that "multiline expansion" in
> context of macros doesn't make any sense, given macros always
> expand to a single line:
> 
>  #define SAME_LINE				\
>  	(__LINE__				\
>  	 == __LINE__)
> 
>  static_assert (SAME_LINE, "");

Sure.

> > Nope, it doesn't warn (neither C nor C++).  I should probably add this test.
> 
> Thanks for confirming.  A test would be nice, to make sure we 
> don't regress.

I'll post a new version with the warning renamed and the new test added.

Thanks,

	Marek