<div dir="rtl"><div dir="ltr">Better do it right on the first time. Some questions about llvm::equal :</div><div dir="ltr"><br></div><div dir="ltr">1) Would STLExtras be good place for llvm::equal ?</div><div dir="ltr"><br></div><div dir="ltr">2) Should its contents be #ifdefed to _MSC_VER, something like</div><div dir="ltr"><br></div><div dir="ltr"><span style="font-size:12.8000001907349px"> template <class I1, class I2></span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px"> bool equal(I1 First1, I1 Last1, I2 First2) {</span></div><div dir="ltr">#ifdef _MSC_VER<br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px"> for (; First1 != Last1; ++First1, ++First2)</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px"> if (!(*First1 == *First2))</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px"> return false;</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px"> return true;</span></div><div dir="ltr">#else</div><div dir="ltr"> return std::equal(First1, Last1, First2);</div><div dir="ltr">#endif<br><span style="font-size:12.8000001907349px"> }</span><br></div><div dir="ltr"><span style="font-size:12.8000001907349px"><br></span></div><div dir="ltr"><span style="font-size:12.8000001907349px">3) Should it replace the other uses of std:;equal in LLVM and clang, although a nullptr is not possible in some (most?) of the code?</span></div><div dir="ltr"><span style="font-size:12.8000001907349px"><br></span></div><div dir="ltr"><span style="font-size:12.8000001907349px"><br></span></div><div dir="ltr"><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-08-12 21:56 GMT+03:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I would still be confused by that code, since I've been using<br>
`std::equal()` for a decade, and it's always worked for me. I<br>
think many newcomers (that aren't used to MSVC's quirks) would,<br>
after sorting out what the checks mean, remove them.<br>
<br>
I think it's better to add a `llvm::equal()` that actually matches<br>
the standard (and, ideally, defers to the STL when it's written<br>
correctly) and use it.<br>
<br>
IMO it would be better to have the correct algorithm inline here<br>
than to have extra (supposed-to-be-unnecessary) checks -- for me,<br>
it would be less cognitive load. (But I'd prefer adding an<br>
`llvm::equal()`.)<br>
<br>
(BTW, I'm okay with you committing the fix that Hans LGTM'ed, as<br>
long as you add a FIXME explaining what the checks are for and<br>
come back soon to clean it up...)<br>
<div class="HOEnZb"><div class="h5"><br>
> On 2015-Aug-12, at 11:31, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
><br>
> Is it clearer to test for no elements rather than begin being a nullptr?<br>
><br>
> bool StructType::isLayoutIdentical(StructType *Other) const {<br>
> if (this == Other) return true;<br>
><br>
> if (isPacked() != Other->isPacked() ||<br>
> getNumElements() != Other->getNumElements())<br>
> return false;<br>
> if (!getNumElements())<br>
> return true;<br>
> return std::equal(element_begin(), element_end(), Other->element_begin());<br>
> }<br>
><br>
><br>
><br>
> 2015-08-12 3:42 GMT+03:00 Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>>:<br>
><br>
> > On 2015-Aug-11, at 14:52, Hans Wennborg via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> ><br>
> > I think it's well defined and supposed to return true, but I'm not C++<br>
> > expert enough to say that with any authority :-)<br>
> ><br>
> > On Tue, Aug 11, 2015 at 2:15 PM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
> >> I'm not sure how std::equal of two nullptrs should be, is this documented or<br>
> >> UB?<br>
> >><br>
> >> Anyhow the question is what to return for the various combinatios of<br>
> >> element_begin() and Other->element_begin() being non/nullptr. What you wrote<br>
> >> makes sense, we probably need something like<br>
> >><br>
> >> if (!element_begin())<br>
> >> return true;<br>
> >> else<br>
> >> return std::equal(element_begin(), element_end(), Other->element_begin());<br>
> >><br>
> >> (because other cases are already handled in the previous if)<br>
> >><br>
> >> What do you think?<br>
> ><br>
> > That code looks good to me (but drop the else after return).<br>
><br>
> It's strange and a little confusing to do this check, and it looks<br>
> easy for someone to "fix" later.<br>
><br>
> std::equal is fairly straightforward to implement. Should we just<br>
> create a copy in `llvm::` and use it?<br>
><br>
> template <class I1, class I2><br>
> bool equal(I1 First1, I1 Last1, I2 First2) {<br>
> for (; First1 != Last1; ++First1, ++First2)<br>
> if (!(*First1 == *First2))<br>
> return false;<br>
> return true;<br>
> }<br>
> template <class I1, class I2, class Compare><br>
> bool equal(I1 First1, I1 Last1, I2 First2, Compare isEqual) {<br>
> for (; First1 != Last1; ++First1, ++First2)<br>
> if (!isEqual(*First1, *First2))<br>
> return false;<br>
> return true;<br>
> }<br>
><br>
> We could maybe defer to the STL if we're using a library with a<br>
> conforming version.<br>
><br>
> ><br>
> >> Also, how this could be tested?<br>
> ><br>
> > To my surprise, we seem to have some unit tests for the Type class, so<br>
> > we could do this:<br>
> ><br>
> > --- a/unittests/IR/TypesTest.cpp<br>
> > +++ b/unittests/IR/TypesTest.cpp<br>
> > @@ -27,4 +27,11 @@ TEST(TypesTest, StructType) {<br>
> > EXPECT_FALSE(Struct->hasName());<br>
> > }<br>
> ><br>
> > +TEST(TypesTest, LayoutIdenticalEmptyStructs) {<br>
> > + LLVMContext C;<br>
> > +<br>
> > + StructType *Foo = StructType::create(C, "Foo");<br>
> > + StructType *Bar = StructType::create(C, "Bar");<br>
> > + EXPECT_TRUE(Foo->isLayoutIdentical(Bar));<br>
> > +}<br>
> ><br>
> ><br>
> ><br>
> >> 2015-08-12 0:04 GMT+03:00 Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>>:<br>
> >>><br>
> >>> (now cc'ing the new llvm-commits list)<br>
> >>><br>
> >>> On Tue, Aug 11, 2015 at 2:01 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
> >>>> On Tue, Aug 4, 2015 at 8:57 AM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>><br>
> >>>> wrote:<br>
> >>>>> Author: yrnkrn<br>
> >>>>> Date: Tue Aug 4 10:57:04 2015<br>
> >>>>> New Revision: 243996<br>
> >>>>><br>
> >>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=243996&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243996&view=rev</a><br>
> >>>>> Log:<br>
> >>>>> Avoid passing nullptr to std::equal.<br>
> >>>>> As documented in the LLVM Coding Standards, indeed MSVC incorrectly<br>
> >>>>> asserts<br>
> >>>>> on this in Debug mode. This happens when building clang with Visual C++<br>
> >>>>> and<br>
> >>>>> -triple i686-pc-windows-gnu on these clang regression tests:<br>
> >>>>><br>
> >>>>> clang/test/CodeGen/2011-03-08-ZeroFieldUnionInitializer.c<br>
> >>>>> clang/test/CodeGen/empty-union-init.c<br>
> >>>><br>
> >>>> Should we merge this to 3.7?<br>
> >>>><br>
> >>>>> Modified: llvm/trunk/lib/IR/Type.cpp<br>
> >>>>> URL:<br>
> >>>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=243996&r1=243995&r2=243996&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=243996&r1=243995&r2=243996&view=diff</a><br>
> >>>>><br>
> >>>>> ==============================================================================<br>
> >>>>> --- llvm/trunk/lib/IR/Type.cpp (original)<br>
> >>>>> +++ llvm/trunk/lib/IR/Type.cpp Tue Aug 4 10:57:04 2015<br>
> >>>>> @@ -612,7 +612,8 @@ bool StructType::isLayoutIdentical(Struc<br>
> >>>>> getNumElements() != Other->getNumElements())<br>
> >>>>> return false;<br>
> >>>>><br>
> >>>>> - return std::equal(element_begin(), element_end(),<br>
> >>>>> Other->element_begin());<br>
> >>>>> + return element_begin() &&<br>
> >>>>> + std::equal(element_begin(), element_end(),<br>
> >>>>> Other->element_begin());<br>
> >>>><br>
> >>>> Actually, wouldn't std::equal return true if element_begin() and<br>
> >>>> element_end() are both null? It seems the new code will now return<br>
> >>>> false for that case. If two StructTypes both have zero elements,<br>
> >>>> shouldn't they be considered as having identical layout?<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="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div>