This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Higuoxing on Jun 3 2018, 9:59 AM.

Details

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

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 3 2018, 9:59 AM
Higuoxing edited the summary of this revision. (Show Details)Jun 3 2018, 10:01 AM

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.

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.

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 updated this revision to Diff 149766.Jun 4 2018, 7:47 AM
Higuoxing edited the summary of this revision. (Show Details)

Update with test cases :)

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

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

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

This comment was removed by Higuoxing.
rnk added a comment.Jun 4 2018, 9:15 AM

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.

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

Higuoxing updated this revision to Diff 150253.Jun 6 2018, 9:32 PM
Higuoxing added a reviewer: echristo.

update with comments & remove some useless blank lines.

@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#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.

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

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

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 updated this revision to Diff 150465.Jun 8 2018, 2:15 AM

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

+ 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 ↗(On Diff #150465)

LogicalOpParenthesesInMacros should be in LogicalNotParentheses group.

672 ↗(On Diff #150465)

No need

include/clang/Basic/DiagnosticSemaKinds.td
5486–5488 ↗(On Diff #150465)

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
12175–12177

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;

Thanks, let me have a try

Higuoxing updated this revision to Diff 150495.Jun 8 2018, 6:00 AM

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 added inline comments.Jun 8 2018, 6:07 AM
include/clang/Basic/DiagnosticGroups.td
264–265 ↗(On Diff #150465)

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

include/clang/Basic/DiagnosticSemaKinds.td
5487 ↗(On Diff #150495)

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
1 ↗(On Diff #150495)

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 updated this revision to Diff 150507.Jun 8 2018, 6:25 AM
Higuoxing marked 4 inline comments as done.

Hope this not disturb you too much : ) thanks

lebedev.ri added inline comments.Jun 8 2018, 6:28 AM
include/clang/Basic/DiagnosticGroups.td
264–265 ↗(On Diff #150465)

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

lebedev.ri added inline comments.Jun 8 2018, 6:32 AM
test/Sema/logical-op-parentheses-in-macros.c
3–4 ↗(On Diff #150507)

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

Higuoxing marked 2 inline comments as done.Jun 8 2018, 6:36 AM

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 updated this revision to Diff 150517.Jun 8 2018, 7:54 AM

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

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#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 marked 3 inline comments as done.Jun 8 2018, 6:28 PM
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#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

Higuoxing retitled this revision from fix: [Bug 18971] - Missing -Wparentheses warning to [clang] [Bug 18971] - fix Missing -Wparentheses warning.Jun 9 2018, 8:09 AM
Higuoxing edited the summary of this revision. (Show Details)
lebedev.ri retitled this revision from [clang] [Bug 18971] - fix Missing -Wparentheses warning to [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).Jun 9 2018, 8:12 AM
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#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 added a comment.EditedJun 11 2018, 7:33 PM

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 updated this revision to Diff 151290.Jun 13 2018, 7:12 PM

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

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?

dexonsmith added subscribers: arphaman, ahatanak.

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
37 ↗(On Diff #151460)

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); }
52 ↗(On Diff #151460)

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

Higuoxing updated this revision to Diff 151599.Jun 15 2018, 7:29 PM
This comment was removed by Higuoxing.
Higuoxing edited the summary of this revision. (Show Details)Jun 15 2018, 10:35 PM
dexonsmith requested changes to this revision.Jun 16 2018, 6:53 PM
dexonsmith added inline comments.
lib/Sema/SemaExpr.cpp
12306–12311

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?

12320

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.

12322

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

12326

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.

12328

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

12332–12335

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
129

typo: should be "no warning".

This revision now requires changes to proceed.Jun 16 2018, 6:53 PM
Higuoxing updated this revision to Diff 151633.Jun 16 2018, 9:33 PM
Higuoxing added inline comments.Jun 16 2018, 9:57 PM
lib/Sema/SemaExpr.cpp
12306–12311

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!

12320

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

12326

Sorry, what period is missing?

12332–12335

Great! The code is more elegant!

dexonsmith added inline comments.Jun 17 2018, 12:17 PM
lib/Sema/SemaExpr.cpp
12238
  • Please add a period at the end of the sentence.
  • "righ" should be "right".
12245

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

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

12253

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

test/Sema/parentheses.c
17–18

I don't think these comments are useful.

19

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

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

This name is strange. VOID_CAST would be more descriptive.

24

I suggest APPLY_OPS_DIRECTLY.

105–107

I'm not sure this comment is particularly useful.

110

Can you add fix-it CHECKs?

113–120

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.

136

Not a useful comment.

149

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 marked 9 inline comments as done.Jun 17 2018, 8:36 PM
Higuoxing added inline comments.
test/Sema/parentheses.c
20

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

105–107

Yes, I delete them : )

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

113–120

Yes, I delete them

Higuoxing updated this revision to Diff 151654.Jun 17 2018, 9:07 PM
Higuoxing marked an inline comment as done.
Higuoxing added inline comments.Jun 17 2018, 9:17 PM
test/Sema/parentheses.c
110

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()

Higuoxing marked 12 inline comments as done.
Higuoxing removed a reviewer: echristo.

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
12245

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

12256–12257

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

test/Sema/parentheses.c
110

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.

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 updated this revision to Diff 151736.Jun 18 2018, 9:36 AM

test case has been passed

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 added a subscriber: vsapsai.Jul 6 2018, 1:16 PM

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.

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 updated this revision to Diff 166583.Sep 21 2018, 5:29 PM

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

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

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.

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.

Higuoxing abandoned this revision.Oct 31 2018, 3:06 AM

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 reclaimed this revision.Nov 5 2018, 8:51 PM

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

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

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
                                      ^
Higuoxing planned changes to this revision.Dec 1 2018, 11:41 PM