[llvm] r243996 - Avoid passing nullptr to std::equal.

[Hans Wennborg via llvm-commits]

I think it's well defined and supposed to return true, but I'm not C++
expert enough to say that with any authority :-)

On Tue, Aug 11, 2015 at 2:15 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> I'm not sure how std::equal of two nullptrs should be, is this documented or
> UB?
>
> Anyhow the question is what to return for the various combinatios of
> element_begin() and Other->element_begin() being non/nullptr. What you wrote
> makes sense, we probably need something like
>
> if (!element_begin())
>   return true;
> else
>   return std::equal(element_begin(), element_end(), Other->element_begin());
>
> (because other cases are already handled in  the previous if)
>
> What do you think?

That code looks good to me (but drop the else after return).

> Also, how this could be tested?

To my surprise, we seem to have some unit tests for the Type class, so
we could do this:

--- a/unittests/IR/TypesTest.cpp
+++ b/unittests/IR/TypesTest.cpp
@@ -27,4 +27,11 @@ TEST(TypesTest, StructType) {
   EXPECT_FALSE(Struct->hasName());
 }

+TEST(TypesTest, LayoutIdenticalEmptyStructs) {
+  LLVMContext C;
+
+  StructType *Foo = StructType::create(C, "Foo");
+  StructType *Bar = StructType::create(C, "Bar");
+  EXPECT_TRUE(Foo->isLayoutIdentical(Bar));
+}



> 2015-08-12 0:04 GMT+03:00 Hans Wennborg <hans at chromium.org>:
>>
>> (now cc'ing the new llvm-commits list)
>>
>> On Tue, Aug 11, 2015 at 2:01 PM, Hans Wennborg <hans at chromium.org> wrote:
>> > On Tue, Aug 4, 2015 at 8:57 AM, Yaron Keren <yaron.keren at gmail.com>
>> > wrote:
>> >> Author: yrnkrn
>> >> Date: Tue Aug  4 10:57:04 2015
>> >> New Revision: 243996
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=243996&view=rev
>> >> Log:
>> >> Avoid passing nullptr to std::equal.
>> >> As documented in the LLVM Coding Standards, indeed MSVC incorrectly
>> >> asserts
>> >> on this in Debug mode. This happens when building clang with Visual C++
>> >> and
>> >> -triple i686-pc-windows-gnu on these clang regression tests:
>> >>
>> >>  clang/test/CodeGen/2011-03-08-ZeroFieldUnionInitializer.c
>> >>  clang/test/CodeGen/empty-union-init.c
>> >
>> > Should we merge this to 3.7?
>> >
>> >> Modified: llvm/trunk/lib/IR/Type.cpp
>> >> URL:
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=243996&r1=243995&r2=243996&view=diff
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/IR/Type.cpp (original)
>> >> +++ llvm/trunk/lib/IR/Type.cpp Tue Aug  4 10:57:04 2015
>> >> @@ -612,7 +612,8 @@ bool StructType::isLayoutIdentical(Struc
>> >>        getNumElements() != Other->getNumElements())
>> >>      return false;
>> >>
>> >> -  return std::equal(element_begin(), element_end(),
>> >> Other->element_begin());
>> >> +  return element_begin() &&
>> >> +         std::equal(element_begin(), element_end(),
>> >> Other->element_begin());
>> >
>> > Actually, wouldn't std::equal return true if element_begin() and
>> > element_end() are both null? It seems the new code will now return
>> > false for that case. If two StructTypes both have zero elements,
>> > shouldn't they be considered as having identical layout?
>
>