<div dir="rtl"><div dir="ltr">Here is a patch for your review including the function, testcase and revisions from Hans and documentation.</div><div dir="ltr"><br></div><div dir="ltr">Regarding the function name, I tried to implement llvm::equal, but this causes ambiguity between std::equal and llvm::equal in std::vector implementation when its type is llvm::something. It uses equal without namespace qualification. Any way around this?</div><div dir="ltr"><br></div><div dir="ltr">If not, llvm::Equal or llvm::isEqual could be used.</div><div dir="ltr"><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-08-12 23:07 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-12, at 12:31, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
><br>
> Getting it right sgtm :-)<br>
><br>
> This isn't the first time we've run into MSVC's broken std::equal, so<br>
> getting a good fix would be nice.<br>
><br>
> (See e.g. <a href="http://llvm.org/viewvc/llvm-project?rev=215988&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215988&view=rev</a><br>
<br>
</span>Ironically, this one was me :/.<br>
<div class="HOEnZb"><div class="h5"><br>
> <a href="http://llvm.org/viewvc/llvm-project?rev=230972&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=230972&view=rev</a><br>
> <a href="http://llvm.org/viewvc/llvm-project?rev=230920&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=230920&view=rev</a>)<br>
><br>
> On Wed, Aug 12, 2015 at 12:22 PM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
>> Better do it right on the first time. Some questions about llvm::equal :<br>
>><br>
>> 1) Would STLExtras be good place for llvm::equal ?<br>
>><br>
>> 2) Should its contents be #ifdefed to _MSC_VER, something like<br>
>><br>
>> template <class I1, class I2><br>
>> bool equal(I1 First1, I1 Last1, I2 First2) {<br>
>> #ifdef _MSC_VER<br>
>> for (; First1 != Last1; ++First1, ++First2)<br>
>> if (!(*First1 == *First2))<br>
>> return false;<br>
>> return true;<br>
>> #else<br>
>> return std::equal(First1, Last1, First2);<br>
>> #endif<br>
>> }<br>
>><br>
>> 3) Should it replace the other uses of std:;equal in LLVM and clang,<br>
>> although a nullptr is not possible in some (most?) of the code?<br>
>><br>
>><br>
>><br>
>><br>
>> 2015-08-12 21:56 GMT+03:00 Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>>:<br>
>>><br>
>>> 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>
>>><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(),<br>
>>>> Other->element_begin());<br>
>>>> }<br>
>>>><br>
>>>><br>
>>>><br>
>>>> 2015-08-12 3:42 GMT+03:00 Duncan P. N. Exon Smith<br>
>>>> <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>>:<br>
>>>><br>
>>>>> On 2015-Aug-11, at 14:52, Hans Wennborg via llvm-commits<br>
>>>>> <<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>><br>
>>>>> wrote:<br>
>>>>>> I'm not sure how std::equal of two nullptrs should be, is this<br>
>>>>>> 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<br>
>>>>>> 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(),<br>
>>>>>> 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>><br>
>>>>>>> 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<br>
>>>>>>>>> incorrectly<br>
>>>>>>>>> asserts<br>
>>>>>>>>> on this in Debug mode. This happens when building clang with<br>
>>>>>>>>> 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>
>>>>>>>>><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>
>>>>>>>>> ==============================================================================<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>
>><br>
<br>
</div></div></blockquote></div><br></div>