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
