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

[Yaron Keren via llvm-commits]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150812/083a0fb1/attachment-0001.html>