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

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

I would still be confused by that code, since I've been using
`std::equal()` for a decade, and it's always worked for me.  I
think many newcomers (that aren't used to MSVC's quirks) would,
after sorting out what the checks mean, remove them.

I think it's better to add a `llvm::equal()` that actually matches
the standard (and, ideally, defers to the STL when it's written
correctly) and use it.

IMO it would be better to have the correct algorithm inline here
than to have extra (supposed-to-be-unnecessary) checks -- for me,
it would be less cognitive load.  (But I'd prefer adding an
`llvm::equal()`.)

(BTW, I'm okay with you committing the fix that Hans LGTM'ed, as
long as you add a FIXME explaining what the checks are for and
come back soon to clean it up...)

> On 2015-Aug-12, at 11:31, Yaron Keren <yaron.keren at gmail.com> wrote:
> 
> Is it clearer to test for no elements rather than begin being a nullptr?
> 
> bool StructType::isLayoutIdentical(StructType *Other) const {
>   if (this == Other) return true;
>   
>   if (isPacked() != Other->isPacked() ||
>       getNumElements() != Other->getNumElements())
>     return false;  
>   if (!getNumElements())
>     return true;
>   return std::equal(element_begin(), element_end(), Other->element_begin());
> }
> 
> 
> 
> 2015-08-12 3:42 GMT+03:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
> 
> > 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
> 
>