[llvm] r243996 - Avoid passing nullptr to std::equal.
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 16 17:48:03 PDT 2015
It would be sad to use different naming :(. Is there a funny
namespace trick we can do?
I can't remember what effect this has:
namespace llvm {
namespace algorithm {
int foo() { /*...*/ }
}
using algorithm::foo;
}
but maybe it's the one we're looking for?
Does anyone else know?
> On 2015-Aug-14, at 23:32, Yaron Keren <yaron.keren at gmail.com> wrote:
>
> Here is a patch for your review including the function, testcase and revisions from Hans and documentation.
>
> Regarding the function name, I tried to implement llvm::equal, but this causes ambiguity between std::equal and llvm::equal in std::vector implementation when its type is llvm::something. It uses equal without namespace qualification. Any way around this?
>
> If not, llvm::Equal or llvm::isEqual could be used.
>
>
> 2015-08-12 23:07 GMT+03:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
>
> > 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
> >>>>
> >>>>
> >>>
> >>
>
>
> <equal.diff>
More information about the llvm-commits
mailing list