<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Mar 14, 2013, at 8:47 AM, Alexander Zinenko <<a href="mailto:ftynse@gmail.com">ftynse@gmail.com</a>> wrote:</div><blockquote type="cite"><div dir="ltr"><div style="">Thanks for review!</div><div style=""><br></div><div style="">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 style="">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 style="">Processing multiple base paths made checking a bit heavier, but I see no other approach.</div></div></blockquote><div><br></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><br></div><div>Also, I think this needs to use isCompleteDefinition().</div></div><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; position: static; z-index: auto; ">
<div class="im">On Mar 12, 2013, at 10:01 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">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 style="">We only need reinterpret-primary-base now.</div></div></div></div></blockquote><div><br></div>Let's call it -Wreinterpret-base-class, actually.</div><div><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; position: static; z-index: auto; ">
+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 style="">Fixed. Shouldn't we provide some details on what causes the difference?</div></div></div></div></blockquote><div><br></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> </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; position: static; z-index: auto; ">
+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 style="">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; position: static; z-index: auto; ">
<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 style="">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; position: static; z-index: auto; ">
<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 style="">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>That's fine.</div><div><br></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; position: static; z-index: auto; ">
<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 style="">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>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><div><br></div><div>John.</div></body></html>