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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Aug-11, at 14:52, Hans Wennborg via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 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).

It's strange and a little confusing to do this check, and it looks
easy for someone to "fix" later.

std::equal is fairly straightforward to implement.  Should we just
create a copy in `llvm::` and use it?

    template <class I1, class I2>
    bool equal(I1 First1, I1 Last1, I2 First2) {
      for (; First1 != Last1; ++First1, ++First2)
        if (!(*First1 == *First2))
          return false;
      return true;
    }
    template <class I1, class I2, class Compare>
    bool equal(I1 First1, I1 Last1, I2 First2, Compare isEqual) {
      for (; First1 != Last1; ++First1, ++First2)
        if (!isEqual(*First1, *First2))
          return false;
      return true;
    }

We could maybe defer to the STL if we're using a library with a
conforming version.

> 
>> 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?
>> 
>> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits