Page MenuHomePhabricator

-Wcomma, a new warning for questionable uses of the comma operator
ClosedPublic

Authored by rtrieu on May 30 2014, 8:03 PM.

Details

Summary

-Wcomma emits a warning when there is a questionable use of the comma operator. It does this by only allowing certain expressions on the LHS of the comma operator, with all other expressions giving a warning. The current whitelisted expressions are increments, decrements, assignments, compound assignments, overloaded versions of these operators, and void returning functions. Some examples of code that will be detected:

int foo();
if (foo(), 5) {} // should be "=="
cout << "foo is " , foo();  // should be "<<"

void bar(int);
void bar(int, int);
bar((foo(), foo()));  // Too many parens, calls the one argument function

Diff Detail

Event Timeline

rtrieu updated this revision to Diff 9977.May 30 2014, 8:03 PM
rtrieu retitled this revision from to -Wcomma, a new warning for questionable uses of the comma operator.
rtrieu updated this object.
rtrieu edited the test plan for this revision. (Show Details)
rtrieu added a subscriber: Unknown Object (MLST).
rtrieu updated this revision to Diff 10379.Jun 12 2014, 10:17 PM

Add a note to the warning that it can be silenced by casting the left hand expression to void.
Catch a few more cases involving templates.

rnk added a subscriber: rnk.Jun 30 2014, 4:38 PM

How do we feel about -Wcomma? I've heard people complain that GCC's -Wparentheses is not very self-documenting.

include/clang/Sema/Sema.h
3626 ↗(On Diff #10379)

micro nit: extra blank line

lib/Sema/SemaExpr.cpp
8610

typo assignemtn

8615

Any reason to only look at the RHS?

8683

Can you explain why a bit? I'm assuming the answer is that people use the comma in macros to simulate multiple statements, and they end up passing the result as a function argument.

8687

Micro nit: end the sentence with a period.

8687

Can you elaborate a tiny bit? I'm guessing the idea here is to diagnose once prior to instantiation, even if there are some things that we can't diagnose without instantiating:

template <typename T>
struct A {
  void foo(int x) {}
  void bar() {
    foo((T::baz(), T::qux()));
  }
};
rtrieu updated this revision to Diff 11006.Jul 1 2014, 1:46 PM
rtrieu added inline comments.Jul 1 2014, 2:07 PM
include/clang/Sema/Sema.h
3626 ↗(On Diff #10379)

Randomly selected one of the two lines and removed it.

lib/Sema/SemaExpr.cpp
8610

Unscrambled the letters.

8615
`-BinaryOperator : outer
  |-BinaryOperator : inner
  | |-LHS
  | `-RHS
  `-RHS : outer

Both BinaryOperators are comma operators.

Currently, this is processing the the outer BinaryOperator and encounters the inner BinaryOperator. We recurse down the RHS to find which expression will be used. The LHS will be checked when processing the inner BinaryOperator.

8683

It is common practice to ignore warnings in macros. One common case is macros which need to return a value, but also perform a logging or checking operation at the same time.

#define CHECK_AND_RETURN_VALUE(x) \
  (log(x), x)

void bar(int num) {
  foo(CHECK_AND_RETURN_VALUE(num));
}
8687

Period added.

8687

This prevents one warning in each template instantiation. Instead, only one warning will be given on the template definition. For cases that are ambiguous, the warning assumes that type-dependent comma operators are bad and warns on them.

rtrieu updated this revision to Diff 12720.Aug 20 2014, 5:13 PM

Add another exemption for RAII objects in the LHS of the comma operator.

rtrieu updated this revision to Diff 13857.Sep 18 2014, 6:11 PM

Removed the false positives by disabling this warning for the increment expression of a for-loop and in template instantiations. Further testing showed that shrinking the whitelist caught mainly more harmless uses of the comma operator.

This would've found the bug fixed by https://codereview.chromium.org/678193003 . It'd be nice to have this :-)

rnk accepted this revision.Dec 3 2014, 11:05 AM
rnk added a reviewer: rnk.

lgtm

Sorry if this got stuck waiting on me...

This revision is now accepted and ready to land.Dec 3 2014, 11:05 AM
rtrieu updated this revision to Diff 21170.Mar 3 2015, 8:17 PM
rtrieu edited edge metadata.

Updated to be based on newer revision.
Remove most cases of the white list. The only cases that remain are casts to void and the third argument in a for-loop.

Did this ever land? Looks like it's lgtm'd and ready to go, and it looks like a useful warning.

Not sure what happened. Will check it out on Monday.

rtrieu added a subscriber: rtrieu.Feb 5 2016, 3:22 PM

So, this week got consumed by -Wconstant-conversion. We'll see how next
week goes.

This revision was automatically updated to reflect the committed changes.

It seems this CL broke LLDB CMake build bot - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/11554
Could you take a look?

rsmith added a subscriber: rsmith.Feb 18 2016, 4:54 PM

This was already fixed in r261285.