<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 21, 2015 at 1:22 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@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 class="HOEnZb"><div class="h5"><br>
> On 2015-Aug-21, at 13:19, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Fri, Aug 21, 2015 at 12:16 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2015-Aug-21, at 10:37, David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> ><br>
> > Author: dblaikie<br>
> > Date: Fri Aug 21 12:37:41 2015<br>
> > New Revision: 245712<br>
> ><br>
> > URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D245712-26view-3Drev&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=vftFLnHiqThJHdL0qZWd_Vo12qdMVOZDFnNVBhP9GKA&m=iFuuRajlg8CK1MZggc-FONg4WvEb9dEMjTcf2snpqjE&s=wcuxTBHNGsUIr6klIeRbCLTEYihMxpwXPg8mbp6uLCY&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D245712-26view-3Drev&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=vftFLnHiqThJHdL0qZWd_Vo12qdMVOZDFnNVBhP9GKA&m=iFuuRajlg8CK1MZggc-FONg4WvEb9dEMjTcf2snpqjE&s=wcuxTBHNGsUIr6klIeRbCLTEYihMxpwXPg8mbp6uLCY&e=</a><br>
> > Log:<br>
> > Remove an unnecessary use of pointee types introduced in r194220<br>
> ><br>
> > David Majnemer (the original author) believes this to be an impossible<br>
> > condition to reach anyway, and no test cases cover this so we'll go with<br>
> > that.<br>
> ><br>
> > Modified:<br>
> > llvm/trunk/lib/IR/ConstantFold.cpp<br>
> ><br>
> > Modified: llvm/trunk/lib/IR/ConstantFold.cpp<br>
> > URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_IR_ConstantFold.cpp-3Frev-3D245712-26r1-3D245711-26r2-3D245712-26view-3Ddiff&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=vftFLnHiqThJHdL0qZWd_Vo12qdMVOZDFnNVBhP9GKA&m=iFuuRajlg8CK1MZggc-FONg4WvEb9dEMjTcf2snpqjE&s=9qg0E1KghzGtl2ckpYoi8CzXjWBQthtOX6aXtRz5Jh8&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_IR_ConstantFold.cpp-3Frev-3D245712-26r1-3D245711-26r2-3D245712-26view-3Ddiff&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=vftFLnHiqThJHdL0qZWd_Vo12qdMVOZDFnNVBhP9GKA&m=iFuuRajlg8CK1MZggc-FONg4WvEb9dEMjTcf2snpqjE&s=9qg0E1KghzGtl2ckpYoi8CzXjWBQthtOX6aXtRz5Jh8&e=</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/lib/IR/ConstantFold.cpp (original)<br>
> > +++ llvm/trunk/lib/IR/ConstantFold.cpp Fri Aug 21 12:37:41 2015<br>
> > @@ -1999,9 +1999,8 @@ static bool isInBoundsIndices(ArrayRef<I<br>
> > /// \brief Test whether a given ConstantInt is in-range for a SequentialType.<br>
> > static bool isIndexInRangeOfSequentialType(SequentialType *STy,<br>
> > const ConstantInt *CI) {<br>
> > - if (auto *PTy = dyn_cast<PointerType>(STy))<br>
> > - // Only handle pointers to sized types, not pointers to functions.<br>
> > - return PTy->getElementType()->isSized();<br>
> > + if (isa<PointerType>(STy))<br>
> > + return true;<br>
><br>
> Worth a comment in the code?<br>
><br>
> Not sure if a comment would add value - the existing comment was just to describe a special case that didn't really exist/couldn't come up, so it seemed like without that the code would be fine as-is.<br>
><br>
> But sure, added in r245730<br>
<br>
</div></div>If you don't think it adds value, then feel free to revert; </blockquote><div><br>I think it adds a bit - I tend to be a bit... terse, when it comes to code, so I usually appreciate the prodding to be more explanatory.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I was<br>
just wondering if someone reading the code would ask the same<br>
question you did originally (and worry that a case was missing).<br></blockquote><div><br>I don't /think/ so, but I could be wrong. I've commented the intent without calling out teh special case, which I don't /think/ would occur to anyone looking at the code, but I could be wrong.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> // And indicies are valid when indexing along a pointer<br>
> if (isa<PointerType>(STy))<br>
> return true;<br>
><br>
><br>
><br>
> ><br>
> > uint64_t NumElements = 0;<br>
> > // Determine the number of elements in our sequential type.<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=vftFLnHiqThJHdL0qZWd_Vo12qdMVOZDFnNVBhP9GKA&m=iFuuRajlg8CK1MZggc-FONg4WvEb9dEMjTcf2snpqjE&s=CrF61doRZdRF05PkR5nctXedsIRfGq3hcZrivnRnW1A&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=vftFLnHiqThJHdL0qZWd_Vo12qdMVOZDFnNVBhP9GKA&m=iFuuRajlg8CK1MZggc-FONg4WvEb9dEMjTcf2snpqjE&s=CrF61doRZdRF05PkR5nctXedsIRfGq3hcZrivnRnW1A&e=</a><br>
<br>
</div></div></blockquote></div><br></div></div>