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

[Higuoxing]

Summary

Hi,

clang currently will not check logical-op-parentheses in macros, it will just skip the checking. And @dexonsmith gave us the explanation

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:

However, sometimes we indeed need the parentheses checking in macros. So, we make the parentheses checking smarter. For details, see my test cases : )

Thanks for reviewing :)

[Higuoxing]

As for some test cases,

$ cat a.cc

#include <iostream>
#include <cassert>

#define bar(x) \
  ( \
    ( std::cout << x ) \
  )

bool x;
int val;

void foo(bool b) {
  std::cout << b << std::endl;
}

int main () {


  foo(x && val == 4 || (!x && val == 5));
  bar(x && val == 4 || (!x && val == 5));
  assert(x && val == 4 || (!x && val == 5));
  assert(x || val == 4 && "test");
  return 0;
}

And clang will emits

a.cc:20:9: warning: '&&' within '||' [-Wlogical-op-parentheses]
  foo(x && val == 4 || (!x && val == 5));
      ~~^~~~~~~~~~~ ~~
a.cc:20:9: note: place parentheses around the '&&' expression to silence this warning
  foo(x && val == 4 || (!x && val == 5));
        ^
      (            )
a.cc:21:9: warning: '&&' within '||' [-Wlogical-op-parentheses]
  bar(x && val == 4 || (!x && val == 5));
  ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
a.cc:7:20: note: expanded from macro 'bar'
    ( std::cout << x ) \
      ~~~~~~~~~~~~~^
a.cc:21:9: note: place parentheses around the '&&' expression to silence this warning
  bar(x && val == 4 || (!x && val == 5));
  ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
a.cc:7:20: note: expanded from macro 'bar'
    ( std::cout << x ) \
      ~~~~~~~~~~~~~^
a.cc:22:12: warning: '&&' within '||' [-Wlogical-op-parentheses]
  assert(x && val == 4 || (!x && val == 5));
         ~~^~~~~~~~~~~ ~~
/usr/include/assert.h:93:25: note: expanded from macro 'assert'
    (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) : (void)0)
                        ^
a.cc:22:12: note: place parentheses around the '&&' expression to silence this warning
  assert(x && val == 4 || (!x && val == 5));
         ~~^~~~~~~~~~~
/usr/include/assert.h:93:25: note: expanded from macro 'assert'
    (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) : (void)0)
                        ^
3 warnings generated.

[lebedev.ri]

Tests?

[lebedev.ri]

In D47687#1120213, @Higuoxing wrote:

As for some test cases,

The tests need to be within the patch itself, in this case
i guess that should go into clang/test/Sema/.
See other tests in there on how to write them.
And they will be run via $ ninja check-clang / $ ninja check-clang-sema.

[Higuoxing]

In D47687#1120226, @lebedev.ri wrote:

In D47687#1120213, @Higuoxing wrote:

As for some test cases,

The tests need to be within the patch itself, in this case
i guess that should go into clang/test/Sema/.
See other tests in there on how to write them.
And they will be run via $ ninja check-clang / $ ninja check-clang-sema.

Hi,

Thanks for your reviewing :D I am trying to write the test cases ...

Thanks :D

[Higuoxing]

Update with test cases :)

[lebedev.ri]

In D47687#1120971, @Higuoxing wrote:

Update with test cases :)

Thanks.

I do believe we are being overly forgiving for macros.
Sometimes that makes sense, sometimes not, especially there is no pre-existing test.

In other words i think this patch has a point.
But i'll leave the actual review for others..

[lebedev.ri]

It seems it was rL119537 that regressed it.
Nice, such a change, no review, no test :)

[dblaikie]

Probably CC someone from apple here & ask about rdar://8678458 - they can
look it up & provide the missing context.

[Higuoxing]

This comment was removed by Higuoxing.

[rnk]

I'm in favor of this direction. The var || var && truth_constant pattern match seems much more robust than the macro pattern match. It's also consistent with what we do outside of macros, so it's less special and surprising.

[Higuoxing]

Thanks for reviewing! I think enabling parentheses-checking in macros could be helpful, and assert(something) is widely used : )

[Higuoxing]

update with comments & remove some useless blank lines.

[rnk]

@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?

[dexonsmith]

In 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.

[Higuoxing]

In 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 D47687#1125964, @dexonsmith wrote:

In 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 :-)

[lebedev.ri]

In D47687#1126032, @Higuoxing wrote:

In 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 D47687#1125964, @dexonsmith wrote:

In 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.

[Higuoxing]

In D47687#1126074, @lebedev.ri wrote:

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.

Yes, that's exactly what I am thinking just now! I think I could make another new patch for this warning ... and then we could determine on which one to take

Thanks : )

[Higuoxing]

I step out a little further... I made an attempt to add a new warning [-Wlogical-op-parentheses-in-macros]. Comparing to the previous one, which do you prefer? I am new to llvm community, and as you see, this is my first patch ... so, forgive me if I made something wrong : ) suggestions are welcoming!

PS. I see something like (x | y & z) checking disabled in macros as well, how to deal with this?

thanks

[lebedev.ri]

+ there will need to be a new standalone test for -Wlogical-op-parentheses-in-macros,
what it does, how it relates to -Wlogical-op-parentheses,-Wparentheses, etc.

include/clang/Basic/DiagnosticGroups.td

264–265

LogicalOpParenthesesInMacros should be in LogicalNotParentheses group.

673

No need

include/clang/Basic/DiagnosticSemaKinds.td

5486–5488

This should be

def warn_logical_and_in_logical_or_in_macros :
  Warning<warn_logical_and_in_logical_or.Text>,
  InGroup<LogicalOpParenthesesInMacros>, DefaultIgnore;

DefaultIgnore is my guess.

lib/Sema/SemaExpr.cpp

12174–12185

Might be simpler to do

Self.Diag(Bop->getOperatorLoc(), OpLoc.isMacroID() ? diag::warn_logical_and_in_logical_or_in_macros :
                                                     diag::warn_logical_and_in_logical_or)
  << Bop->getSourceRange() << OpLoc;

[Higuoxing]

Thanks, let me have a try

[Higuoxing]

Update without conflict with test case parentheses.c... I add a separate option [-Wlogical-op-parentheses-in-macros] and will be silence by default

thanks

[lebedev.ri]

include/clang/Basic/DiagnosticGroups.td

264–265

That should have of course been: LogicalOpParenthesesInMacros should be in LogicalOpParentheses group.

include/clang/Basic/DiagnosticSemaKinds.td

5487

More than 80 chars per line.

def warn_logical_and_in_logical_or_in_macros: Warning<
  warn_logical_and_in_logical_or.Text>, InGroup<LogicalOpParenthesesInMacros>,
  DefaultIgnore;

test/Sema/logical-op-parentheses-in-macros.c

2

You need to also test that it is not enabled by default,
and is enabled by -Wlogical-op-parentheses.
It can sill be in one file, see e.g. clang/test/Sema/tautological-unsigned-enum-zero-compare.c for examples.

[Higuoxing]

Hope this not disturb you too much : ) thanks

[lebedev.ri]

include/clang/Basic/DiagnosticGroups.td

264–265

Are you sure you are uploading the correct diff?
Neither of these two notes is done.

[lebedev.ri]

test/Sema/logical-op-parentheses-in-macros.c

4–5

The comment got lost, but these two are incorrect, they don't actually do anything, the -verify= is missing.

[Higuoxing]

Sorry, I will check it and update the test case

Besides, shall I add the macro-parentheses checking test cases to the original 'parentheses.c'?

[Higuoxing]

This diff version pass the both parentheses.c and logical-op-parentheses.c

[dexonsmith]

In D47687#1126074, @lebedev.ri wrote:

In D47687#1126032, @Higuoxing wrote:

In 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 D47687#1125964, @dexonsmith wrote:

In 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.

[Higuoxing]

In D47687#1126607, @dexonsmith wrote:

In D47687#1126074, @lebedev.ri wrote:

In D47687#1126032, @Higuoxing wrote:

In 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 D47687#1125964, @dexonsmith wrote:

In 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?

Thanks

[dexonsmith]

In D47687#1127120, @Higuoxing wrote:

In D47687#1126607, @dexonsmith wrote:

In D47687#1126074, @lebedev.ri wrote:

In D47687#1126032, @Higuoxing wrote:

In 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 D47687#1125964, @dexonsmith wrote:

In 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.

[Higuoxing]

In 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.

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!

[Higuoxing]

Ping, well, I think I understand @dexonsmith's idea.

Actionable macro argument is just like something like this

#define foo(x) ( (void)(x) )

when we using it by

foo(a && b || c);

we could add parentheses for it by

foo((a && b) || c);

However, the following example is not actionable

#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )

when we using it by

foo(&&, ||, x, y, z);

because, we also probably use it by

foo(+, *, x, y, z)

So, we cannot add parentheses for it carelessly...

My opinion is that, to judge if an expr is actionable is to test if the op and both LHS and RHS are from same context or same argument position from macro... But I cannot find such API for indicating a expression expanded position exactly. There are API like: isMacroArgExpansion and isMacroBodyExpansion which is determined by row string positions...

So, what I can do now for this is that using API isMacroArgExpansion() to let it check the parentheses defined or expanded totally inside a macro like

#define foo(x, y, z) ( x && y || z )

The shortage is that we cannot check parentheses for this case:

foo(x && y || z);

because the operator is expanded from macro arguments.

Thanks

[Higuoxing]

I think I am almost there.

Here, In my sight

#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )

is un-actionable, because x op0 y op1 z are from different arguments of function-like macro, so, we will not check parentheses for op0 or op1 when we call it by

foo(&&, ||, x, y, z)

but if we call it by

foo(&&, ||, x && y ||z, y, z)

it is actionable, because argument (x && y || z) is from the same arg position of macro foo;
hence we should check x && y || z but shouldn't check parentheses for the first argument (&&) and second argument (||)

I think this could cover bunch of cases. But I think my code is not so beautiful... So, is there any suggestion?

[Higuoxing]

Ping

[dexonsmith]

In D47687#1133222, @Higuoxing wrote:

I think I am almost there.

Here, In my sight

#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )

is un-actionable, because x op0 y op1 z are from different arguments of function-like macro, so, we will not check parentheses for op0 or op1 when we call it by

foo(&&, ||, x, y, z)

but if we call it by

foo(&&, ||, x && y ||z, y, z)

it is actionable, because argument (x && y || z) is from the same arg position of macro foo;
hence we should check x && y || z but shouldn't check parentheses for the first argument (&&) and second argument (||)

SGTM!

I think this could cover bunch of cases. But I think my code is not so beautiful... So, is there any suggestion?

I made a couple of comments on the tests, but I'd appreciate someone else reviewing the code. @arphaman? @ahatanak?

test/Sema/logical-op-parentheses-in-macros.c

29

Phabricator seems to be reporting that there's a missing newline at the end of the file.

38

Please also check that there's no warning with nested macros:

#define VOID(cond) (void)(cond)
#define BUILD_VOID(op1, op2, x, y, z) VOID(x op1 y op2 z)

void foo(unsigned i) { BUILD_VOID(&&, ||, i, i, i); }

[Higuoxing]

This comment was removed by Higuoxing.

[dexonsmith]

lib/Sema/SemaExpr.cpp

12314–12319

It seems unfortunate not to update this one at the same time. Or are you planning that for a follow-up?

Can you think of a way to share the logic cleanly?

12328

I think the code inside this should be split out into a separate function (straw man name: "diagnoseLogicalAndInLogicalOr") so that you can use early returns and reduce nesting.

12330

I don't think this comment is valuable. It's just repeating the code in the condition.

12334

This comment is just repeating what's in the condition. I suggest replacing it with something describing the logic that follows. Also, it's missing a period at the end of the sentence.

12336

This will be cleaner if you create a local reference to Self.getSourceManager() called SM.

12340–12343

It's a little awkward to add this condition in with the previous one, and you're also repeating a call to isMacroArgExpansion unnecessarily. I suggest something like:

SourceLocation OpExpansion;
if (!SM.isMacroArgExpansion(OpLoc, &OpExpansion))
  return;

SourceLocation ExprExpansion;
if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansion) &&
    OpExpansion == ExprExpansion)
  DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);

if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), &ExprExpansion) &&
    OpExpansion == ExprExpansion)
  DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);

test/Sema/parentheses.c

133 ↗

(On Diff #151605)

typo: should be "no warning".

[Higuoxing]

lib/Sema/SemaExpr.cpp

12314–12319

Yes, I would like to update it after this patch being accepted, because I think this patch is about logical-op-parentheses and this one is about bitwise-op, and I think after this patch being accepted I will be more confident on the code style, API using and various things : ) Of course, I would like to help!

12328

Yes, I add a function DiagnoseLogicalAndInLogicalOr, does the uppercase D matter?

12334

Sorry, what period is missing?

12340–12343

Great! The code is more elegant!

[dexonsmith]

lib/Sema/SemaExpr.cpp

12246

• Please add a period at the end of the sentence.
• "righ" should be "right".

12253

You can reduce nesting below by adding an early return above this.

I also think you should describe at a high-level what you're trying to do in the code that follows. Something like:

// Only diagnose operators in macros if they're from the same argument position.

12259

This comment is just describing what's clear from the code. I think you should drop it, and the similar one later.

12261

This line has odd indentation. Please run clang-format-diff.py to clean up the patch.

test/Sema/parentheses.c

17–18 ↗

(On Diff #151633)

I don't think these comments are useful.

19 ↗

(On Diff #151633)

Can you combine this with NON_NESTING_VOID_0 (which I think should be called VOID_CAST) below?

20 ↗

(On Diff #151633)

• You haven't actually wrapped NESTING_VOID here.
• A more descriptive name would be APPLY_OPS or something.

23 ↗

(On Diff #151633)

This name is strange. VOID_CAST would be more descriptive.

24 ↗

(On Diff #151633)

I suggest APPLY_OPS_DIRECTLY.

109–111 ↗

(On Diff #151633)

I'm not sure this comment is particularly useful.

114 ↗

(On Diff #151633)

Can you add fix-it CHECKs?

117–124 ↗

(On Diff #151633)

I think this comment should be fairly well implied by the commit and commit message the test is part of. I don't think it's necessary.

140 ↗

(On Diff #151633)

Not a useful comment.

153 ↗

(On Diff #151633)

I don't think checking within other macro arguments is necessary here. You have a combinatorial explosion of tests, but it seems unlikely code would be written in such a way as to make this wrong.

I would like to see tests like the following:

NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning.

[Higuoxing]

test/Sema/parentheses.c

20 ↗

(On Diff #151633)

Yes, I made a crucial mistake! So, I make a little adjustment here, we will check parentheses for expressions, if and only if the operator and its LHS, RHS are from same arg position of a *non-nesting macro*, because it seems very difficult to distinguish a nesting macro's args are from same arg position of its father macro

109–111 ↗

(On Diff #151633)

Yes, I delete them : )

114 ↗

(On Diff #151633)

llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses around the '&&' expression to silence this warning
  VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
            ~~^~~~
llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro 'VOID_CAST'
#define VOID_CAST(cond) ( (void)(cond) )
                                 ^~~~

Sorry, it seems that when deal with expressions in macros, there is no fix-it hint ...

117–124 ↗

(On Diff #151633)

Yes, I delete them

[Higuoxing]

test/Sema/parentheses.c

114 ↗

(On Diff #151633)

The suggestParentheses suppress the fix-it hint when the expression is in macros

if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
    EndLoc.isValid()) {
  Self.Diag(Loc, Note)
    << FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
    << FixItHint::CreateInsertion(EndLoc, ")");
} else {
  // We can't display the parentheses, so just show the bare note.
  Self.Diag(Loc, Note) << ParenRange;
}

You see, there is a isFileID()

[dexonsmith]

Did you miss this comment from my previous review?

I would like to see tests like the following:

NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning.

lib/Sema/SemaExpr.cpp

12253

You added an early return -- but then you didn't actually reduce the nesting. Please remove the else that follows.

12264–12265

Can you reverse these two checks? The second one looks cheaper.

test/Sema/parentheses.c

114 ↗

(On Diff #151633)

Can you make it work? The diagnostic was disabled because it was low quality: no fix-it, and firing when it was not actionable. I'm not convinced we should turn it back on until we can give a fix-it.

[Higuoxing]

In D47687#1135284, @dexonsmith wrote:

Did you miss this comment from my previous review?

I would like to see tests like the following:

NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning.

I think this test case should pass, please wait
I cloned a newer version codes, and I am compiling it... about 1-2 hours

Sorry for disturbing you too much on reviewing codes

I think I could give it a try on giving it a fix-it...

Big thanks!

[Higuoxing]

test case has been passed

[Higuoxing]

Sorry, It seems a little bit difficult for me to add a proper fix-it hint for expressions in macros, because I cannot find the exact position of the last char of the token on right hand side of operator. Any suggestion?

Actually, in gcc, it will emit warning for everything without a fix-it hint, let alone expressions in nested macros... I think that our warning note is enough for us to diagnose the lacking parentheses, as you see in my inline comment

llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses around the '&&' expression to silence this warning
  VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
            ~~^~~~
llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro 'VOID_CAST'
#define VOID_CAST(cond) ( (void)(cond) )
                                 ^~~~

So, it depends on you whether the parentheses checking in macros should be reserved. Both OK for me... Thanks for your reviewing :)

[vsapsai]

In D47687#1136351, @Higuoxing wrote:

Sorry, It seems a little bit difficult for me to add a proper fix-it hint for expressions in macros, because I cannot find the exact position of the last char of the token on right hand side of operator. Any suggestion?

Can you please clarify what exactly isn't working? SuggestParentheses is doing most of what you need but in this case it doesn't work because of macro locations:

static void SuggestParentheses(Sema &Self, SourceLocation Loc,
                               const PartialDiagnostic &Note,
                               SourceRange ParenRange) {
  SourceLocation EndLoc = Self.getLocForEndOfToken(ParenRange.getEnd());
  if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
      EndLoc.isValid()) {
    Self.Diag(Loc, Note)
      << FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
      << FixItHint::CreateInsertion(EndLoc, ")");
  } else {
    // We can't display the parentheses, so just show the bare note.
    Self.Diag(Loc, Note) << ParenRange;
  }
}

Hope this gives you an idea how you can try to make fix-its work.

[Higuoxing]

In D47687#1154707, @vsapsai wrote:

Hope this gives you an idea how you can try to make fix-its work.

Hi, Thank you @vsapsai , Yes, I am afraid if I add some extra function and may cause some problems. Because, this API getLocForEndOfToken seems not supported in macros. And I am not so familiar with sourcelocation system in clang, I will try my best to achieve this. Thanks a lot

[Higuoxing]

I update the *SuggestParentheses* function, now, it could deal with parentheses inside macros

[Higuoxing]

Ping. Now, this patch could give fix-it note on parentheses in macros.

[vsapsai]

Sorry about the delay. The change seems to be correct but ninja check-clang reveals the test "Misc/caret-diags-macros.c" is failing. Can you please look into that?

Appreciate your contribution, Xing, and thanks for verifying your change with tests.

lib/Sema/SemaExpr.cpp

7049–7050

Can you please start variables with an upper case letter? Like SpellLoc, TokLen.

[Higuoxing]

In D47687#1266893, @vsapsai wrote:

Sorry about the delay. The change seems to be correct but ninja check-clang reveals the test "Misc/caret-diags-macros.c" is failing. Can you please look into that?

Hi, It's because the expression is given in multiple lines and my SuggestParentheses cannot give proper fix-it hint. But I could give proper ParenRange in macros. Shall we reserve current SuggestParentheses and just give Parenrange hit in macros ? I have no idea about this ...

Appreciate your contribution, Xing, and thanks for verifying your change with tests.

My pleasure :) Thanks for your review.

[vsapsai]

Sorry, you've decided to abandon the patch, it took a lot of good work. Xing, are you sure you don't want to see this change finished? I agree that delays in code review can be frustrating and I think it is something we can improve.

[Higuoxing]

In D47687#1288272, @vsapsai wrote:

Sorry, you've decided to abandon the patch, it took a lot of good work. Xing, are you sure you don't want to see this change finished?

No, I am working on this :)

I agree that delays in code review can be frustrating and I think it is something we can improve.

Never mind :)

Thank you, I am still working on this :) I am afraid this open revision will take up your unnecessary reviewing time or waste your reviewing time, so I close this revision temporarily ... Now, reopen this revision :) Just give me some time :)

[vsapsai]

OK. Good to know you are still working on it.

In D47687#1271880, @Higuoxing wrote:

In D47687#1266893, @vsapsai wrote:

Sorry about the delay. The change seems to be correct but ninja check-clang reveals the test "Misc/caret-diags-macros.c" is failing. Can you please look into that?

Hi, It's because the expression is given in multiple lines and my SuggestParentheses cannot give proper fix-it hint. But I could give proper ParenRange in macros. Shall we reserve current SuggestParentheses and just give Parenrange hit in macros ? I have no idea about this ...

Why is the expression on multiple lines in this case? I didn't check in debugger but the test case looks like you can place ) between BAD_CONDITIONAL_OPERATOR and ;. At least that's what I would expect as compiler user.

llvm-project/clang/test/Misc/caret-diags-macros.c:125:38: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first
int test4 = BAD_CONDITIONAL_OPERATOR+BAD_CONDITIONAL_OPERATOR;
            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
llvm-project/clang/test/Misc/caret-diags-macros.c:124:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR'
#define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
                                      ^
llvm-project/clang/test/Misc/caret-diags-macros.c:125:38: note: place parentheses around the '+' expression to silence this warning
llvm-project/clang/test/Misc/caret-diags-macros.c:124:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR'
#define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
                                      ^
llvm-project/clang/test/Misc/caret-diags-macros.c:125:38: note: place parentheses around the '?:' expression to evaluate it first
int test4 = BAD_CONDITIONAL_OPERATOR+BAD_CONDITIONAL_OPERATOR;
                                     ^
                                     (
llvm-project/clang/test/Misc/caret-diags-macros.c:124:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR'
#define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
                                      ^

include/clang/Basic/DiagnosticGroups.td

def CXX2aCompatPedantic : DiagGroup<"c++2a-compat-pedantic",
                                     [CXX2aCompat]>;
 
 def ExitTimeDestructors : DiagGroup<"exit-time-destructors">;
 def FlexibleArrayExtensions : DiagGroup<"flexible-array-extensions">;
 def FourByteMultiChar : DiagGroup<"four-char-constants">;
 def GlobalConstructors : DiagGroup<"global-constructors">;
 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
+def LogicalOpParenthesesInMacros: DiagGroup<"logical-op-parentheses-in-macros", 
+                                            [LogicalOpParentheses]>;
 def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
 def ExpansionToDefined : DiagGroup<"expansion-to-defined">;
 def FlagEnum : DiagGroup<"flag-enum">;

 
 // -Widiomatic-parentheses contains warnings about 'idiomatic'
 // missing parentheses;  it is off by default.  We do not include it
 // in -Wparentheses because most users who use -Wparentheses explicitly
 // do not want these warnings.
 def ParenthesesOnEquality : DiagGroup<"parentheses-equality">;
 def Parentheses : DiagGroup<"parentheses",
                             [LogicalOpParentheses,
                              LogicalNotParentheses,
                              BitwiseOpParentheses,
                              ShiftOpParentheses,
                              OverloadedShiftOpParentheses,
                              ParenthesesOnEquality,
                              DanglingElse]>;
 
 // -Wconversion has its own warnings, but we split a few out for
 // legacy reasons:

include/clang/Basic/DiagnosticSemaKinds.td

def note_logical_instead_of_bitwise_remove_constant : Note<
   "remove constant to silence this warning">;
 
 def warn_bitwise_op_in_bitwise_op : Warning<
   "'%0' within '%1'">, InGroup<BitwiseOpParentheses>;
 
 def warn_logical_and_in_logical_or : Warning<
   "'&&' within '||'">, InGroup<LogicalOpParentheses>;
 
+def warn_logical_and_in_logical_or_in_macros: 
+  Warning<warn_logical_and_in_logical_or.Text>, 
+  InGroup<LogicalOpParenthesesInMacros>, DefaultIgnore;
+
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
   InGroup<OverloadedShiftOpParentheses>;
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
 def warn_addition_in_bitshift : Warning<

lib/Sema/SemaExpr.cpp

 /// SuggestParentheses - Emit a note with a fixit hint that wraps
 /// ParenRange in parentheses.
 static void SuggestParentheses(Sema &Self, SourceLocation Loc,
                                const PartialDiagnostic &Note,
                                SourceRange ParenRange) {
   SourceLocation EndLoc = Self.getLocForEndOfToken(ParenRange.getEnd());
   if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
       EndLoc.isValid()) {
     Self.Diag(Loc, Note)
       << FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
       << FixItHint::CreateInsertion(EndLoc, ")");
   } else {
     // We can't display the parentheses, so just show the bare note.
     Self.Diag(Loc, Note) << ParenRange;
   }
 }
 
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {

 }
 
 /// It accepts a '&&' expr that is inside a '||' one.
 /// Emit a diagnostic together with a fixit hint that wraps the '&&' expression
 /// in parentheses.
 static void
 EmitDiagnosticForLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,
                                        BinaryOperator *Bop) {
   assert(Bop->getOpcode() == BO_LAnd);
-  Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or)
-      << Bop->getSourceRange() << OpLoc;
+  Self.Diag(Bop->getOperatorLoc(), !OpLoc.isMacroID() ?
+    // if this warning is in a normal context
+    diag::warn_logical_and_in_logical_or :
+    // else this warning is in a macro context
+    // currently, this will not warn in macros by default.
+    // add [-Wlogical-op-parentheses-in-macros]
+    // to enable this warning.
+    diag::warn_logical_and_in_logical_or_in_macros
+  ) << Bop->getSourceRange() << OpLoc;
+  
   SuggestParentheses(Self, Bop->getOperatorLoc(),
     Self.PDiag(diag::note_precedence_silence)
       << Bop->getOpcodeStr(),
     Bop->getSourceRange());
 }
 
 /// Returns true if the given expression can be evaluated as a constant
 /// 'true'.
 static bool EvaluatesAsTrue(Sema &S, Expr *E) {

 static void DiagnoseLogicalAndInLogicalOrLHS(Sema &S, SourceLocation OpLoc,
                                              Expr *LHSExpr, Expr *RHSExpr) {
   if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(LHSExpr)) {
     if (Bop->getOpcode() == BO_LAnd) {
       // If it's "a && b || 0" don't warn since the precedence doesn't matter.
       if (EvaluatesAsFalse(S, RHSExpr))
         return;
       // If it's "1 && a || b" don't warn since the precedence doesn't matter.
+      // And 'assert("some message" && a || b)' don't warn as well.
       if (!EvaluatesAsTrue(S, Bop->getLHS()))
         return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
     } else if (Bop->getOpcode() == BO_LOr) {
       if (BinaryOperator *RBop = dyn_cast<BinaryOperator>(Bop->getRHS())) {
         // If it's "a || b && 1 || c" we didn't warn earlier for
         // "a || b && 1", but warn now.
         if (RBop->getOpcode() == BO_LAnd && EvaluatesAsTrue(S, RBop->getRHS()))
           return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, RBop);
       }
     }
   }
 }
 
 /// Look for '&&' in the right hand of a '||' expr.
 static void DiagnoseLogicalAndInLogicalOrRHS(Sema &S, SourceLocation OpLoc,
                                              Expr *LHSExpr, Expr *RHSExpr) {
   if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(RHSExpr)) {
     if (Bop->getOpcode() == BO_LAnd) {
       // If it's "0 || a && b" don't warn since the precedence doesn't matter.
       if (EvaluatesAsFalse(S, LHSExpr))
         return;
       // If it's "a || b && 1" don't warn since the precedence doesn't matter.
+      // And 'assert(a || b && "some message")' don't warn as well.
       if (!EvaluatesAsTrue(S, Bop->getRHS()))
         return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
     }
   }
 }
 
 /// Look for bitwise op in the left or right hand of a bitwise op with
 /// lower precedence and emit a diagnostic together with a fixit hint that wraps
 /// the '&' expression in parentheses.
 static void DiagnoseBitwiseOpInBitwiseOp(Sema &S, BinaryOperatorKind Opc,
                                          SourceLocation OpLoc, Expr *SubExpr) {
   if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(SubExpr)) {
     if (Bop->isBitwiseOp() && Bop->getOpcode() < Opc) {
       S.Diag(Bop->getOperatorLoc(), diag::warn_bitwise_op_in_bitwise_op)
         << Bop->getOpcodeStr() << BinaryOperator::getOpcodeStr(Opc)
         << Bop->getSourceRange() << OpLoc;
       SuggestParentheses(S, Bop->getOperatorLoc(),
         S.PDiag(diag::note_precedence_silence)
           << Bop->getOpcodeStr(),
         Bop->getSourceRange());
     }
   }
 }
 
 static void DiagnoseAdditionInShift(Sema &S, SourceLocation OpLoc,
                                     Expr *SubExpr, StringRef Shift) {
   if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(SubExpr)) {
     if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
       StringRef Op = Bop->getOpcodeStr();
       S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
           << Bop->getSourceRange() << OpLoc << Shift << Op;
       SuggestParentheses(S, Bop->getOperatorLoc(),
           S.PDiag(diag::note_precedence_silence) << Op,
           Bop->getSourceRange());

 /// precedence.
 static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
                                     SourceLocation OpLoc, Expr *LHSExpr,
                                     Expr *RHSExpr){
   // Diagnose "arg1 'bitwise' arg2 'eq' arg3".
   if (BinaryOperator::isBitwiseOp(Opc))
     DiagnoseBitwisePrecedence(Self, Opc, OpLoc, LHSExpr, RHSExpr);
 
   // Diagnose "arg1 & arg2 | arg3"
   if ((Opc == BO_Or || Opc == BO_Xor) &&
       !OpLoc.isMacroID()/* Don't warn in macros. */) {
     DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, LHSExpr);
     DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, RHSExpr);
   }
 
   // Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
-  // We don't warn for 'assert(a || b && "bad")' since this is safe.
-  if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
+  // Here we will not skip 'logical and in logical or' checking
+  // in macros, since 'assert(a || b && "some message")' is equal
+  // to '(a || b && 1)' and 'assert("some message" && a || b)' is
+  // equal to '(1 && a || b)'.
+  if (Opc == BO_LOr) {
     DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
     DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
   }
 
   if ((Opc == BO_Shl && LHSExpr->getType()->isIntegralType(Self.getASTContext()))
       || Opc == BO_Shr) {
     StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
     DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
     DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
   }
 
   // Warn on overloaded shift operators and comparisons, such as:
   // cout << 5 == 4;
   if (BinaryOperator::isComparisonOp(Opc))
     DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);
 }
 
 // Binary Operators.  'Tok' is the token for the operator.
 ExprResult Sema::ActOnBinOp(Scope *S, SourceLocation TokLoc,
                             tok::TokenKind Kind,
                             Expr *LHSExpr, Expr *RHSExpr) {
   BinaryOperatorKind Opc = ConvertTokenKindToBinaryOpcode(Kind);
   assert(LHSExpr && "ActOnBinOp(): missing left expression");
   assert(RHSExpr && "ActOnBinOp(): missing right expression");
 

test/Sema/logical-op-parentheses-in-macros.c

+// RUN: %clang_cc1 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -Wlogical-op-parentheses-in-macros -fsyntax-only \
+// RUN:            -verify=logical-op-parentheses %s
+// RUN: %clang_cc1 -Wlogical-op-parentheses -fsyntax-only \
+// RUN:            -verify=silence %s
+
+#define macro_op_parentheses_check(x) ( \
+  ( (void)(x) ) \
+)
+
+// silence-no-diagnostics
+void logical_op_in_macros_test(unsigned i) {
+  macro_op_parentheses_check(i || i && i); // logical-op-parentheses-warning {{'&&' within '||'}} \
+                                           // logical-op-parentheses-note {{place parentheses around the '&&' expression to silence this warning}}
+
+  macro_op_parentheses_check(i || i && "I love LLVM"); // no warning.
+  macro_op_parentheses_check("I love LLVM" && i || i); // no warning.
+
+  macro_op_parentheses_check(i || i && "I love LLVM" || i); // logical-op-parentheses-warning {{'&&' within '||'}} \
+                                                            // logical-op-parentheses-note {{place parentheses around the '&&' expression to silence this warning}}
+
+  macro_op_parentheses_check(i || "I love LLVM" && i || i); // logical-op-parentheses-warning {{'&&' within '||'}} \
+                                                            // logical-op-parentheses-note {{place parentheses around the '&&' expression to silence this warning}}
+
+  macro_op_parentheses_check(i && i || 0); // no warning.
+  macro_op_parentheses_check(0 || i && i); // no warning.
+}