[llvm] r243996 - Avoid passing nullptr to std::equal.
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 12 12:31:19 PDT 2015
Getting it right sgtm :-)
This isn't the first time we've run into MSVC's broken std::equal, so
getting a good fix would be nice.
(See e.g. http://llvm.org/viewvc/llvm-project?rev=215988&view=rev
http://llvm.org/viewvc/llvm-project?rev=230972&view=rev
http://llvm.org/viewvc/llvm-project?rev=230920&view=rev)
On Wed, Aug 12, 2015 at 12:22 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> 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
>> >
>> >
>>
>
More information about the llvm-commits
mailing list