[llvm] r243996 - Avoid passing nullptr to std::equal.
Yaron Keren via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 14 23:32:23 PDT 2015
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
> >>>>
> >>>>
> >>>
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150815/f119060a/attachment-0001.html>
-------------- next part --------------
Index: docs/CodingStandards.rst
===================================================================
--- docs/CodingStandards.rst (revision 245034)
+++ docs/CodingStandards.rst (working copy)
@@ -179,7 +179,7 @@
missing. Fortunately, they are rarely needed.
* The locale support is incomplete.
* ``std::equal()`` (and other algorithms) incorrectly assert in MSVC when given
- ``nullptr`` as an iterator.
+ ``nullptr`` as an iterator. Use ``llvm::Equal`` instead.
Other than these areas you should assume the standard library is available and
working as expected until some build bot tells you otherwise. If you're in an
Index: include/llvm/ADT/ArrayRef.h
===================================================================
--- include/llvm/ADT/ArrayRef.h (revision 245034)
+++ include/llvm/ADT/ArrayRef.h (working copy)
@@ -12,6 +12,7 @@
#include "llvm/ADT/None.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/STLExtras.h"
#include <vector>
namespace llvm {
@@ -156,9 +157,7 @@
bool equals(ArrayRef RHS) const {
if (Length != RHS.Length)
return false;
- if (Length == 0)
- return true;
- return std::equal(begin(), end(), RHS.begin());
+ return llvm::Equal(begin(), end(), RHS.begin());
}
/// slice(n) - Chop off the first N elements of the array.
Index: include/llvm/ADT/STLExtras.h
===================================================================
--- include/llvm/ADT/STLExtras.h (revision 245034)
+++ include/llvm/ADT/STLExtras.h (working copy)
@@ -371,6 +371,20 @@
std::forward<UnaryPredicate>(P));
}
+#ifdef _MSC_VER
+// Visual C++ 2013 asserts on valid input of First1 = Last1 = nullptr.
+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;
+}
+#else
+template <class I1, class I2> bool Equal(I1 First1, I1 Last1, I2 First2) {
+ return std::equal(First1, Last1, First2);
+}
+#endif
+
//===----------------------------------------------------------------------===//
// Extra additions to <memory>
//===----------------------------------------------------------------------===//
Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAG.cpp (revision 245034)
+++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp (working copy)
@@ -17,6 +17,7 @@
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
@@ -5668,7 +5669,7 @@
"Update with wrong number of operands");
// If no operands changed just return the input node.
- if (Ops.empty() || std::equal(Ops.begin(), Ops.end(), N->op_begin()))
+ if (llvm::Equal(Ops.begin(), Ops.end(), N->op_begin()))
return N;
// See if the modified node already exists.
Index: lib/IR/Instruction.cpp
===================================================================
--- lib/IR/Instruction.cpp (revision 245034)
+++ lib/IR/Instruction.cpp (working copy)
@@ -11,6 +11,7 @@
//
//===----------------------------------------------------------------------===//
+#include "llvm/ADT/STLExtras.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/CallSite.h"
#include "llvm/IR/Constants.h"
@@ -343,12 +344,12 @@
// We have two instructions of identical opcode and #operands. Check to see
// if all operands are the same.
- if (!std::equal(op_begin(), op_end(), I->op_begin()))
+ if (!llvm::Equal(op_begin(), op_end(), I->op_begin()))
return false;
if (const PHINode *thisPHI = dyn_cast<PHINode>(this)) {
const PHINode *otherPHI = cast<PHINode>(I);
- return std::equal(thisPHI->block_begin(), thisPHI->block_end(),
+ return llvm::Equal(thisPHI->block_begin(), thisPHI->block_end(),
otherPHI->block_begin());
}
Index: lib/IR/Type.cpp
===================================================================
--- lib/IR/Type.cpp (revision 245034)
+++ lib/IR/Type.cpp (working copy)
@@ -14,6 +14,7 @@
#include "llvm/IR/Type.h"
#include "LLVMContextImpl.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/IR/Module.h"
#include <algorithm>
#include <cstdarg>
@@ -610,8 +611,7 @@
getNumElements() != Other->getNumElements())
return false;
- return element_begin() &&
- std::equal(element_begin(), element_end(), Other->element_begin());
+ return llvm::Equal(element_begin(), element_end(), Other->element_begin());
}
/// getTypeByName - Return the type with the specified name, or null if there
Index: unittests/IR/TypesTest.cpp
===================================================================
--- unittests/IR/TypesTest.cpp (revision 245034)
+++ unittests/IR/TypesTest.cpp (working copy)
@@ -27,4 +27,12 @@
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));
+}
+
} // end anonymous namespace
More information about the llvm-commits
mailing list