[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D47687#1127120, @Higuoxing wrote:

> In https://reviews.llvm.org/D47687#1126607, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote:
> >
> > > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote:
> > >
> > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> > > >
> > > > > @dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like `assert(x || y && "str");`, if so we can go forward with this.
> > > > >
> > > > > @chandlerc I know you've hit this behavior difference vs. GCC before. Any thoughts on the proposed change?
> > > >
> > > >
> > > >
> > > >
> > > > In https://reviews.llvm.org/D47687#1125964, @dexonsmith wrote:
> > > >
> > > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> > > > >
> > > > > > @dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like `assert(x || y && "str");`, if so we can go forward with this.
> > > > >
> > > > >
> > > > > There were two commits with this radar: r119537 and r119540.  The main motivation was a deeply nested macro that when "inlined" included the moral equivalent of `#define DOUBLE_OP(OP1, OP2, X, Y, Z) (X OP1 Y OP2 Z)`.  There was terrible note spew when the warning fired in this case, and the use case for the macro made the warning un-actionable.  We decided to suppress the warning entirely:
> > > > >
> > > > > > As a general comment, the warning seems to be useless for macros; I'll follow the example of warn_logical_instead_of_bitwise which doesn't trigger for macros and I'll make the warning not warn for macros.
> > > >
> > > >
> > > > Hi, Thank you,
> > > >
> > > > I noticed that `warn_logical_instead_of_bitwise ` will also skip parentheses checking in macros... well, this patch seems not so necessary... both ok for me ... depends on all of you :-)
> > >
> > >
> > > At worst, we can issue this warning in a new `-Wparentheses-in-macros` subgroup, which, if apple so insists, could be off-by-default.
> > >  That would less worse than just completely silencing it for the entire world.
> >
> >
> > I’d be fine with strengthening the existing warning as long as there is an actionable fix-it.  I suspect if you suppress it when the relevant expression is constructed from multiple macro arguments that will be good enough.
>
>
> Thanks, currently, `[-Wparentheses | -Wlogical-op-parentheses]` will not emit warning for parentheses in macros. only if you add `[-Wlogical-op-parentheses-in-macros]` it will emit something like `'&&' within '||'` warning...
>
> However, `'&' within '|'` checking was disabled in macros as well... I don't know if this patch meet the needs... if this patch was ok, then, just as @lebedev.ri said, Maybe we could add a `[-Wparentheses-in-macros]` subgroup and add these warning into this new group, in the future... depends on users :-) any suggestion?


Yes, I think understand the patch; but I think it's the wrong direction.  I think we should just make the existing `-Wlogical-op-parentheses` smart enough to show actionable warnings in macros (but suppress the ones that are not actionable, like the internals of `foo(&&, ||, ...)`, rather than adding `-Wlogical-op-parentheses-in-macros`, which sounds like it would be permanently off-by-default.


https://reviews.llvm.org/D47687