<div dir="ltr">Hello, John!<div><br></div><div>Thanks for your comments and sorry that it's taking so long to polish my code.</div><div style>Please find the updated version attached.</div><div class="gmail_extra"><br>
<div class="gmail_quote">On 20 March 2013 07:02, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="im"><div>On Mar 14, 2013, at 8:47 AM, Alexander Zinenko <<a href="mailto:ftynse@gmail.com" target="_blank">ftynse@gmail.com</a>> wrote:</div><blockquote type="cite">
<div dir="ltr"><div>Thanks for review!</div><div><br></div><div>I attached a quiet version of a patch that doesn't warn if any of the paths is a non-virtually-derived base at offset zero. </div>
<div>During first iteration, Jordan proposed to make a separate switch to enable a noisy version that warns even if the cast is safe, though. If it is still required, it can be easily added back.</div><div>Processing multiple base paths made checking a bit heavier, but I see no other approach.</div>
</div></blockquote><div><br></div></div><div><div>+ if(!SrcRD || !SrcRD->hasDefinition())</div><div>+ return;</div><div>+</div><div>+ const CXXRecordDecl *DestRD = DestType->getPointeeCXXRecordDecl();</div><div>
+</div><div>+ if(!DestRD || !DestRD->hasDefinition())</div><div>+ return;</div><div><br></div><div>Please add a comment saying that we're deliberately not instantiating</div><div>templates here because we're not allowed to.</div>
</div></div></div></blockquote><div style>Actually, we do not call CheckReinterpretCast if the cast is type-dependent. The reason behind these checks is the following case with forward declaration</div><div style>class A;</div>
<div style>class B {};</div><div style>void foo(A *a) {</div><div style> reinterpret_cast<B *>(a);</div><div style>}</div><div style>in which we can't examine the definition of A since we don't have it yet.</div>
<div style>I added a comment and mentioned templates at the end though.</div><div style><br></div><div style>--</div><div style>Alex</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div><br></div><div>Also, I think this needs to use isCompleteDefinition().</div></div><div class="im"><br><blockquote type="cite"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote">On 12 March 2013 20:14, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>On Mar 12, 2013, at 10:01 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:<br>
> Pinging this myself, adding Richard Smith as another reviewer. I don't quite feel qualified to give the final word on it, but I'm fixing an analyzer bug that's basically "don't crash when the user does this in exactly the right wrong way", and it'd be nice to have a warning so that they fix their code.<br>
<br>
</div>+def ReinterpretUpDownCastZeroAdjustment :<br>
+ DiagGroup<"reinterpret-updown-zero-adjustment">;<br>
+def ReinterpretUpDownCast : DiagGroup<"reinterpret-updown",<br>
<br>
How about -Wreinterpret-base and -Wreinterpret-primary-base?</blockquote><div>We only need reinterpret-primary-base now.</div></div></div></div></blockquote><div><br></div></div>Let's call it -Wreinterpret-base-class, actually.</div>
<div><div class="im"><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+def warn_reinterpret_wrong_subobject : Warning<<br>
+ "'reinterpret_cast' might return a pointer to wrong subobject">,<br>
<br>
How about "reinterpret_cast %select{from|to}3 class %0 %select{to|from}3 %select{virtual base|base at non-zero offset}2 %1 behaves differently from static_cast"?<br></blockquote><div>Fixed. Shouldn't we provide some details on what causes the difference?</div>
</div></div></div></blockquote><div><br></div></div><div>Well, my suggestion mentions that it's a virtual base or a base at a non-zero</div><div>offset, but if you want something more or different, feel free.</div><div class="im">
<div> </div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+static void DiagnoseReinterpretUpDownCast(Sema &Self, ExprResult &SrcExpr,<br>
+ QualType DestType,<br>
+ const SourceRange &OpRange) {<br>
<br>
You should take SrcExpr as a const Expr * and OpRange as a SourceRange.<br></blockquote><div>Fixed.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ const Type *SrcUnqualType = SrcType.getTypePtr();<br>
+ const Type *DestUnqualType = DestType.getTypePtr();<br>
<br>
This is unnecessary and also doesn't necessarily actually give you an unqualified type.<br></blockquote><div>Deleted. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ // When casting from pointer or reference, get pointee type; use original<br>
+ // type otherwise.<br>
+ const CXXRecordDecl *SrcPointeeRD = SrcUnqualType->getPointeeCXXRecordDecl();<br>
+ const CXXRecordDecl *SrcRD =<br>
+ SrcPointeeRD ? SrcPointeeRD : SrcUnqualType->getAsCXXRecordDecl();<br>
<br>
It'd be good to remember the original non-pointer sugared types here for diagnostic purposes.<br>
<br>
Also, at this point in semantic analysis you can differentiate between the pointer and reference cases by just checking SrcExpr->isRValue(). </blockquote><div>Warning "wrong cast to T*" looks clearer to me than "wrong cast to T" if the code is reinterpret_cast<T *>(...).</div>
</div></div></div></blockquote><div><br></div></div>That's fine.</div><div><br></div><div><div class="im"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
+ CXXBasePaths SrcBP, DestBP;<br>
+<br>
...<br>
+ const ASTRecordLayout &SrcLayout = Self.Context.getASTRecordLayout(SrcRD);<br>
+ const ASTRecordLayout &DestLayout = Self.Context.getASTRecordLayout(DestRD);<br>
<br>
1. Please sink this down to the point where you need it.<br>
2. Please only actually get the layout for the derived class.</blockquote><div>We actually need the layout of any derived class in the pass to process indirect base offsets correctly.</div></div></div></div></blockquote>
<div><br></div></div>Good point. But we definitely don't need the layout of the base class. :)</div><div><br></div><div><div>+ enum {</div><div>+ ReinterpretNormalCast = -1,</div><div>+ ReinterpretUpcast,</div>
<div>+ ReinterpretDowncast</div><div>+ } ReinterpretKind;</div><div>+ ReinterpretKind = ReinterpretNormalCast;</div><div><br></div></div><div>You don't need ReinterpretNormalCast. Just leave it uninitialized;</div>
<div>any reasonable -Wuninitialized will be able to figure out that it's initialized</div><div>along every path.</div><div><br></div><div>Otherwise, this looks great. Feel free to check in with these changes.</div><span class="HOEnZb"><font color="#888888"><div>
<br></div><div>John.</div></font></span></div>
</blockquote></div><br></div></div>