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

[Reid Kleckner]

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

================
Comment at: include/clang/Sema/Sema.h:3626
@@ -3624,1 +3625,3 @@
+
+
   /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
----------------
micro nit: extra blank line

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

================
Comment at: lib/Sema/SemaExpr.cpp:8615
@@ +8614,3 @@
+      case BO_Comma:
+        return IgnoreCommaOperand(BO->getRHS());
+      case BO_Assign:
----------------
Any reason to only look at the RHS?

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

================
Comment at: lib/Sema/SemaExpr.cpp:8687
@@ +8686,3 @@
+
+  // Don't warn in template instantiations
+  if (!ActiveTemplateInstantiations.empty())
----------------
Micro nit: end the sentence with a period.

================
Comment at: lib/Sema/SemaExpr.cpp:8687
@@ +8686,3 @@
+
+  // Don't warn in template instantiations
+  if (!ActiveTemplateInstantiations.empty())
----------------
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()));
    }
  };

http://reviews.llvm.org/D3976