[PATCH] -Wcomma, a new warning for questionable uses of the comma operator

[Richard Trieu]

================
Comment at: include/clang/Sema/Sema.h:3626
@@ -3624,1 +3625,3 @@
+
+
   /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
----------------
Reid Kleckner wrote:
> micro nit: extra blank line
Randomly selected one of the two lines and removed it.

================
Comment at: lib/Sema/SemaExpr.cpp:8610
@@ +8609,3 @@
+
+  // Allow assignemtn and compound assignments.  Also recurse on other
+  // comma operators.
----------------
Reid Kleckner wrote:
> typo assignemtn
Unscrambled the letters.

================
Comment at: lib/Sema/SemaExpr.cpp:8615
@@ +8614,3 @@
+      case BO_Comma:
+        return IgnoreCommaOperand(BO->getRHS());
+      case BO_Assign:
----------------
Reid Kleckner wrote:
> Any reason to only look at the RHS?
  `-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.

================
Comment at: lib/Sema/SemaExpr.cpp:8683
@@ +8682,3 @@
+void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
+  // No warnings in macros
+  if (Loc.isMacroID())
----------------
Reid Kleckner wrote:
> 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.
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));
  }

================
Comment at: lib/Sema/SemaExpr.cpp:8687
@@ +8686,3 @@
+
+  // Don't warn in template instantiations
+  if (!ActiveTemplateInstantiations.empty())
----------------
Reid Kleckner wrote:
> Reid Kleckner wrote:
> > Micro nit: end the sentence with a period.
> 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()));
>     }
>   };
Period added.

================
Comment at: lib/Sema/SemaExpr.cpp:8687
@@ +8686,3 @@
+
+  // Don't warn in template instantiations
+  if (!ActiveTemplateInstantiations.empty())
----------------
Richard Trieu wrote:
> Reid Kleckner wrote:
> > Reid Kleckner wrote:
> > > Micro nit: end the sentence with a period.
> > 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()));
> >     }
> >   };
> Period added.
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.

http://reviews.llvm.org/D3976