[cfe-commits] [Patch] Handle overloaded operators in warning about conditional operator precedence

[Chandler Carruth]

On Tue, Jun 7, 2011 at 4:37 AM, Hans Wennborg <hans at chromium.org> wrote:

> Thank you very much for the comments, Chandler!


> On Mon, Jun 6, 2011 at 6:55 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
> > I have a few problems with this patch.
> > First, I like breaking up parts of it into simpler code, but
> unfortunately
> > we're duplicating a fair amount of work stripping off various parts of
> the
> > expression. I would lift the stripping logic back into the common
> function
> > so that its only ever done once.
>
> I'm not sure I follow you. The stripping *is* only done once:
> IsArithmeticBinaryExpr works with the condition expression, and
> ExprLooksBoolean works with the right-hand side of it.
>

Ah, I just missed that we only re-process the RHS. My bad.


> > Also, doxyments would be good.
>
> Done. I'm not too familiar with doxygen, so please nag me if I didn't
> get it right.
>

In general I prefer the following form over the '/// nameOfMethod - ...'
form:

/// \brief Checks whether the expression is ...
///
/// This function checks whether the given expression is ....
/// It strips all paren expressions, implicit casts, and conversions on the
way ...
/// \param E The expression processed

It's not a huge deal though.


> This doesn't handle implicit constructors (they're modeled as
> CXXConstructExpr), and I don't think it has to either.
>
> Since this is the condition expression, it has to be implicitly
> convertable to bool, and if I remember correctly, conversions such as
> "x -> implicit constructor -> conversion op" can't happen in C++.
>

Ahh, I see. You're only really trying to look through an implicit conversion
to bool / int / etc. Got it.


A couple of stylistic issues. Once you fix these, feel free to commit.

In a few places the '*' is attached to the type name; they go on the other
side in Clang.

In code like:
+  if (E->getType()->isBooleanType())
+    return true;
+  else if (BinaryOperator *OP = dyn_cast<BinaryOperator>(E))
+    return IsLogicOp(OP->getOpcode());
+  else if (UnaryOperator *OP = dyn_cast<UnaryOperator>(E))
+    return OP->getOpcode() == UO_LNot;

No need to use 'else' here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110609/a96ada43/attachment.html>