On Tue, Jun 7, 2011 at 4:37 AM, Hans Wennborg <[email protected]> wrote:
> Thank you very much for the comments, Chandler! > On Mon, Jun 6, 2011 at 6:55 PM, Chandler Carruth <[email protected]> > 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
