Higuoxing added a comment. In https://reviews.llvm.org/D47687#1128757, @dexonsmith wrote:
> > 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. In https://reviews.llvm.org/D47687#1128757, @dexonsmith wrote: > 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. Thank you `@lebedev.ri` for reviewing my code and change the title and various things! Thank you `@dexonsmith` for helping me on backgrounds of this topic, I do agree with you, because those who did not participate in our discussion might not know this option `[-Wlogical-op-parentheses-in-macros]`, and make a special option for this warning seems a little bit strange ... I will have a try, thanks a lot! https://reviews.llvm.org/D47687 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
