[llvm] r243996 - Avoid passing nullptr to std::equal.
Yaron Keren via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 12 12:22:05 PDT 2015
Better do it right on the first time. Some questions about llvm::equal :
1) Would STLExtras be good place for llvm::equal ?
2) Should its contents be #ifdefed to _MSC_VER, something like
template <class I1, class I2>
bool equal(I1 First1, I1 Last1, I2 First2) {
#ifdef _MSC_VER
for (; First1 != Last1; ++First1, ++First2)
if (!(*First1 == *First2))
return false;
return true;
#else
return std::equal(First1, Last1, First2);
#endif
}
3) Should it replace the other uses of std:;equal in LLVM and clang,
although a nullptr is not possible in some (most?) of the code?
2015-08-12 21:56 GMT+03:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
> 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
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150812/33036651/attachment.html>
More information about the llvm-commits
mailing list