[llvm] r243996 - Avoid passing nullptr to std::equal.
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 12 13:07:46 PDT 2015
> On 2015-Aug-12, at 12:31, Hans Wennborg <hans at chromium.org> wrote:
>
> 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
Ironically, this one was me :/.
> 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