[PATCH] new check checking use of identical expressions inside a conditional expression, i.e. '?'.

[Jordan Rose]

Looking good! But I still think we should just highlight the true and false expressions, not the entire conditional (which may span multiple lines):

+    BR.EmitBasicReport(
+        AC->getDecl(), "Identical expressions in conditional expression",
+        categories::LogicError,
+        "identical expressions on both sides of ':' in conditional expression",
+        ELoc,C->getSourceRange());

You can simply construct a SourceRange[] on the stack and then pass that to the ArrayRef<SourceRange> argument.

More comments:

+  static PathDiagnosticLocation createConditionalColonLoc(
+                                                  const ConditionalOperator *CO,
+                                                  const SourceManager &SM);

I think the new style is to either break after the return type or to double-indent wrapped arguments, rather than right-aligning them. (Right-aligning is harder to maintain.)


 static bool isIdenticalExpr(const ASTContext &Ctx, const Expr *Expr1,
-                            const Expr *Expr2);
+                            const Expr *Expr2,
+                            const bool IgnoreSideEffects = false);

We generally don't bother making parameters const. Pointers-to-const, yes, but not the parameters themselves. (It does help catch stupid mistakes sometimes, but it's not worth being inconsistent with the rest of Clang.)


+  if (isIdenticalExpr(AC->getASTContext(), C->getTrueExpr(),
+                      C->getFalseExpr(),true)) {

Missing space after the comma.


-    if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2))
+    if (!Child1 || !Child2 || !isIdenticalExpr(Ctx, Child1, Child2,
+        IgnoreSideEffects))

Indentation is wrong; the 'IgnoreSideEffects' is an argument to 'isIdenticalExpr', so it should be aligned with the first argument in the call. (I misread this at first as another || possibility.


   case Stmt::ArraySubscriptExprClass:
+  case Stmt::CallExprClass:
   case Stmt::CStyleCastExprClass:
   case Stmt::ImplicitCastExprClass:
   case Stmt::ParenExprClass:

Aren't calls only identical if IgnoreSideEffects is true? (Or if the callee is constexpr or __attribute__((pure)) or ((const)), I suppose, but I'm okay with leaving that out.)

You could argue that calls being identical on both sides of an operator might still be an error because the calls are unsequenced, but I suppose that doesn't matter if you're using == or !=. So I don't think we can treat them as identical here, much as I'd like to, unless we have IgnoreSideEffects.

Please add tests for the comparisons operators involving calls as well.


 PathDiagnosticLocation
+  PathDiagnosticLocation::createConditionalColonLoc(
+                                            const ConditionalOperator *CO,
+                                            const SourceManager &SM) {
+  return PathDiagnosticLocation(CO->getColonLoc(), SM, SingleLocK);
+}

Similar to above: prefer double-indented parameters to right-aligned parameters, and no need for that first indent before the qualified name. (I see other functions in the file are doing that, but not new ones.)


+void test_expr_negative_func() {
+  unsigned a = 0;
+  unsigned b = 1;
+  a = a > 5 ? a+func() : a+func2(); // no warning
+}

Please also include a test where you're calling the same function with different arguments, to demonstrate that we're stepping into the CallExpr.

Thanks!
Jordan


On Nov 27, 2013, at 4:43 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:

> Hi,
>  
> I have updated the check to warn on a++,a=1 and a+func() as the comments suggested.
>  
> /Anders
>  
> > I think we should actually be warning, since only one side of the branch will ever be executing. Maybe we can include a flag here that says whether the expression walker should step into calls? What do you think?
>  
> This sounds reasonable. We could warn about other "non-const" expressions too, I believe:
>  
>    a ? b++ : b++;
>     a ? b=2 : b=2;
>  
> Best regards,
> Daniel Marjamäki
>  
> ..................................................................................................................
> Daniel Marjamäki Senior Engineer
> Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden
>  
> Mobile:                 +46 (0)709 12 42 62
> E-mail:                 Daniel.Marjamaki at evidente.se
>  
> www.evidente.se
>  
> ________________________________________
> Från: Jordan Rose [jordan_rose at apple.com]
> Skickat: den 20 november 2013 18:22
> Till: Per Viberg
> Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
> Ämne: Re: [PATCH] new check checking use of identical expressions inside a conditional expression, i.e. '?'.
>  
> Hm, interesting. In this case:
>  
> +void test_expr_negative_func() {
> +  unsigned a = 0;
> +  unsigned b = 1;
> +  a = a > 5 ? a+func() : a+func(); // no warning
> +}
>  
> I think we should actually be warning, since only one side of the branch will ever be executing. Maybe we can include a flag here that says whether the expression walker should step into calls? What do you think?
>  
> Other than that, this looks good, except that I would add the true-expr and false-expr as ranges to highlight in addition to putting the warning itself on the colon.
>  
> Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131204/45c697c0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: identicalexpr.diff
Type: application/octet-stream
Size: 10150 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131204/45c697c0/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131204/45c697c0/attachment-0001.html>