<div dir="rtl"><div dir="ltr">Is it clearer to test for no elements rather than begin being a nullptr?<br></div><div dir="ltr"><br></div><div dir="ltr"><div dir="ltr">bool StructType::isLayoutIdentical(StructType *Other) const {</div><div dir="ltr"> if (this == Other) return true;</div><div dir="ltr"> </div><div dir="ltr"> if (isPacked() != Other->isPacked() ||</div><div dir="ltr"> getNumElements() != Other->getNumElements())</div><div dir="ltr"> return false; </div><div dir="ltr"> if (!getNumElements())</div><div dir="ltr"> return true;</div><div dir="ltr"> return std::equal(element_begin(), element_end(), Other->element_begin());</div><div dir="ltr">}</div><div><br></div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-08-12 3:42 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"><span class=""><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>
</span>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>
<div><div class="h5"><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>
</div></div>> _______________________________________________<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>
</blockquote></div><br></div>