<div class="gmail_quote">On Tue, Jun 7, 2011 at 4:37 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Thank you very much for the comments, Chandler! </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><br>
On Mon, Jun 6, 2011 at 6:55 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> I have a few problems with this patch.<br>
> First, I like breaking up parts of it into simpler code, but unfortunately<br>
> we're duplicating a fair amount of work stripping off various parts of the<br>
> expression. I would lift the stripping logic back into the common function<br>
> so that its only ever done once.<br>
<br>
</div>I'm not sure I follow you. The stripping *is* only done once:<br>
IsArithmeticBinaryExpr works with the condition expression, and<br>
ExprLooksBoolean works with the right-hand side of it.<br></blockquote><div><br></div><div>Ah, I just missed that we only re-process the RHS. My bad.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
> Also, doxyments would be good.<br>
<br>
</div>Done. I'm not too familiar with doxygen, so please nag me if I didn't<br>
get it right.<br></blockquote><div><br></div><div>In general I prefer the following form over the '/// nameOfMethod - ...' form:</div><div><br></div><div>/// \brief Checks whether the expression is ...</div><div>
///</div><div>/// This function checks whether the given expression is ....</div><div>/// It strips all paren expressions, implicit casts, and conversions on the way ...</div><div>/// \param E The expression processed</div>
<div><br></div><div>It's not a huge deal though.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">This doesn't handle implicit constructors (they're modeled as</div>
CXXConstructExpr), and I don't think it has to either.<br>
<br>
Since this is the condition expression, it has to be implicitly<br>
convertable to bool, and if I remember correctly, conversions such as<br>
"x -> implicit constructor -> conversion op" can't happen in C++.<br></blockquote><div><br></div><div>Ahh, I see. You're only really trying to look through an implicit conversion to bool / int / etc. Got it.</div>
<div><br></div><div><br></div><div>A couple of stylistic issues. Once you fix these, feel free to commit.</div><div><br></div><div>In a few places the '*' is attached to the type name; they go on the other side in Clang.</div>
<div><br></div><div>In code like:</div><div><div>+  if (E->getType()->isBooleanType())</div><div>+    return true;</div><div>+  else if (BinaryOperator *OP = dyn_cast<BinaryOperator>(E))</div><div>+    return IsLogicOp(OP->getOpcode());</div>
<div>+  else if (UnaryOperator *OP = dyn_cast<UnaryOperator>(E))</div><div>+    return OP->getOpcode() == UO_LNot;</div></div><div><br></div><div>No need to use 'else' here.</div></div>