<div dir="ltr"><div style>Hello John,</div><div style><br></div><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>
<br><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 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 style><br></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">
+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><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 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">
<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">
<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><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 style>We actually need the layout of any derived class in the pass to process indirect base offsets correctly.</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">
</blockquote><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">
3. You can re-use the paths object if the first lookup failed.</blockquote><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>
+ if (SrcBP.getDetectedVirtual() || DestBP.getDetectedVirtual()) {<br>
<br>
You are ignoring the possibility that there are subobjects at multiple paths. We should not warn if *any* of the paths is a non-virtually-derived base at offset zero.</blockquote><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">
</blockquote><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>
+ CharUnits Offset = CharUnits::Zero();<br>
+ if (ReinterpretKind == ReinterpretUpcast)<br>
+ Offset = SrcLayout.getBaseClassOffset(DestRD);<br>
+ else if (ReinterpretKind == ReinterpretDowncast)<br>
+ Offset = DestLayout.getBaseClassOffset(SrcRD);<br>
<br>
This won't work for indirect base classes. </blockquote><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>
+ DiagnoseReinterpretUpDownCast(Self, SrcExpr, DestType, OpRange);<br>
+<br>
<br>
Please sink this until we've decided that the cast succeeds<font color="#888888">.</font></blockquote><div style> Fixed all of the above.</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></blockquote><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"><span class=""><font color="#888888">
John.<br>
<br></font></span></blockquote><div style>--</div><div style>Alex </div></div><br></div></div>