[llvm] r336805 - Recommit r334887: [SmallSet] Add SmallSetIterator.
Zachary Turner via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 12 11:32:04 PDT 2018
I was about to revert this too but Galina beat me. The problem is that in
MSVC's STL, set<T>::const_iterator is not trivially copy constructible.
On Wed, Jul 11, 2018 at 11:06 AM Galina Kistanova via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Hello Florian,
>
> This commit broke build step on one of our builders:
>
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/10903/steps/build-unified-tree/logs/stdio
>
> . . .
> FAILED: unittests/ADT/CMakeFiles/ADTTests.dir/SmallSetTest.cpp.obj
> C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe /nologo /TP -DEXPENSIVE_CHECKS
> -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_LANG_CXX11=1 -DUNICODE
> -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS
> -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_DEBUG
> -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS
> -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
> -D__STDC_LIMIT_MACROS -Iunittests\ADT
> -IC:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\unittests\ADT
> -Iinclude
> -IC:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\include
> -IC:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\utils\unittest\googletest\include
> -IC:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\utils\unittest\googlemock\include
> /DWIN32 /D_WINDOWS /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /W4
> -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351
> -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800
> -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706
> -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091
> -wd4592 -wd4319 -wd4324 -w14062 -we4238 /MDd /Zi /Ob0 /Od /RTC1 /EHs-c-
> /GR- /showIncludes
> /Founittests\ADT\CMakeFiles\ADTTests.dir\SmallSetTest.cpp.obj
> /Fdunittests\ADT\CMakeFiles\ADTTests.dir\ /FS -c
> C:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\unittests\ADT\SmallSetTest.cpp
> C:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\unittests\ADT\SmallSetTest.cpp(80):
> error C2280: 'llvm::SmallSetIterator<T,4,C>::SmallSetIterator(const
> llvm::SmallSetIterator<T,4,C> &)': attempting to reference a deleted
> function
> with
> [
> T=int,
> C=std::less<int>
> ]
> C:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\include\llvm/ADT/SmallSet.h(80):
> note: compiler has generated
> 'llvm::SmallSetIterator<T,4,C>::SmallSetIterator' here
> with
> [
> T=int,
> C=std::less<int>
> ]
>
>
> Please have a look?
> The builder was already red and did not send notifications.
>
> Thanks
>
>
> Galina
>
> On Wed, Jul 11, 2018 at 6:39 AM, Florian Hahn via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: fhahn
>> Date: Wed Jul 11 06:39:59 2018
>> New Revision: 336805
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336805&view=rev
>> Log:
>> Recommit r334887: [SmallSet] Add SmallSetIterator.
>>
>> This version now uses the subset of is_trivially_XXX provided by
>> GCC 4.8 and llvm/Support/type_traits.h
>>
>> Modified:
>> llvm/trunk/include/llvm/ADT/SmallSet.h
>> llvm/trunk/unittests/ADT/SmallSetTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/ADT/SmallSet.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallSet.h?rev=336805&r1=336804&r2=336805&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ADT/SmallSet.h (original)
>> +++ llvm/trunk/include/llvm/ADT/SmallSet.h Wed Jul 11 06:39:59 2018
>> @@ -17,14 +17,68 @@
>> #include "llvm/ADT/None.h"
>> #include "llvm/ADT/SmallPtrSet.h"
>> #include "llvm/ADT/SmallVector.h"
>> +#include "llvm/ADT/iterator.h"
>> #include "llvm/Support/Compiler.h"
>> +#include "llvm/Support/type_traits.h"
>> #include <cstddef>
>> #include <functional>
>> #include <set>
>> +#include <type_traits>
>> #include <utility>
>>
>> namespace llvm {
>>
>> +/// SmallSetIterator - This class implements a const_iterator for
>> SmallSet by
>> +/// delegating to the underlying SmallVector or Set iterators.
>> +template <typename T, unsigned N, typename C>
>> +class SmallSetIterator
>> + : public iterator_facade_base<SmallSetIterator<T, N, C>,
>> + std::forward_iterator_tag, T> {
>> +private:
>> + using SetIterTy = typename std::set<T, C>::const_iterator;
>> + using VecIterTy = typename SmallVector<T, N>::const_iterator;
>> + using SelfTy = SmallSetIterator<T, N, C>;
>> +
>> + /// Iterators to the parts of the SmallSet containing the data. They
>> are set
>> + /// depending on isSmall.
>> + union {
>> + SetIterTy SetIter;
>> + VecIterTy VecIter;
>> + };
>> +
>> + bool isSmall;
>> +
>> +public:
>> + SmallSetIterator(SetIterTy SetIter) : SetIter(SetIter), isSmall(false)
>> {
>> + // Use static_assert here, as the SmallSetIterator type is
>> incomplete in the
>> + // class scope.
>> + static_assert(std::is_trivially_destructible<SelfTy>() &&
>> + llvm::is_trivially_copy_constructible<SelfTy>() &&
>> + llvm::is_trivially_move_constructible<SelfTy>(),
>> + "SelfTy needs to by trivially constructible and
>> destructible");
>> + }
>> +
>> + SmallSetIterator(VecIterTy VecIter) : VecIter(VecIter), isSmall(true)
>> {}
>> +
>> + bool operator==(const SmallSetIterator &RHS) const {
>> + if (isSmall != RHS.isSmall)
>> + return false;
>> + if (isSmall)
>> + return VecIter == RHS.VecIter;
>> + return SetIter == RHS.SetIter;
>> + }
>> +
>> + SmallSetIterator &operator++() { // Preincrement
>> + if (isSmall)
>> + VecIter++;
>> + else
>> + SetIter++;
>> + return *this;
>> + }
>> +
>> + const T &operator*() const { return isSmall ? *VecIter : *SetIter; }
>> +};
>> +
>> /// SmallSet - This maintains a set of unique values, optimizing for the
>> case
>> /// when the set is small (less than N). In this case, the set can be
>> /// maintained with no mallocs. If the set gets large, we expand to
>> using an
>> @@ -50,6 +104,7 @@ class SmallSet {
>>
>> public:
>> using size_type = size_t;
>> + using const_iterator = SmallSetIterator<T, N, C>;
>>
>> SmallSet() = default;
>>
>> @@ -121,6 +176,18 @@ public:
>> Set.clear();
>> }
>>
>> + const_iterator begin() const {
>> + if (isSmall())
>> + return {Vector.begin()};
>> + return {Set.begin()};
>> + }
>> +
>> + const_iterator end() const {
>> + if (isSmall())
>> + return {Vector.end()};
>> + return {Set.end()};
>> + }
>> +
>> private:
>> bool isSmall() const { return Set.empty(); }
>>
>>
>> Modified: llvm/trunk/unittests/ADT/SmallSetTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallSetTest.cpp?rev=336805&r1=336804&r2=336805&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/ADT/SmallSetTest.cpp (original)
>> +++ llvm/trunk/unittests/ADT/SmallSetTest.cpp Wed Jul 11 06:39:59 2018
>> @@ -13,6 +13,7 @@
>>
>> #include "llvm/ADT/SmallSet.h"
>> #include "gtest/gtest.h"
>> +#include <string>
>>
>> using namespace llvm;
>>
>> @@ -68,3 +69,57 @@ TEST(SmallSetTest, Erase) {
>>
>> EXPECT_EQ(0u, s1.count(8));
>> }
>> +
>> +TEST(SmallSetTest, IteratorInt) {
>> + SmallSet<int, 4> s1;
>> +
>> + // Test the 'small' case.
>> + for (int i = 0; i < 3; i++)
>> + s1.insert(i);
>> +
>> + std::vector<int> V(s1.begin(), s1.end());
>> + // Make sure the elements are in the expected order.
>> + std::sort(V.begin(), V.end());
>> + for (int i = 0; i < 3; i++)
>> + EXPECT_EQ(i, V[i]);
>> +
>> + // Test the 'big' case by adding a few more elements to switch to
>> std::set
>> + // internally.
>> + for (int i = 3; i < 6; i++)
>> + s1.insert(i);
>> +
>> + V.assign(s1.begin(), s1.end());
>> + // Make sure the elements are in the expected order.
>> + std::sort(V.begin(), V.end());
>> + for (int i = 0; i < 6; i++)
>> + EXPECT_EQ(i, V[i]);
>> +}
>> +
>> +TEST(SmallSetTest, IteratorString) {
>> + // Test SmallSetIterator for SmallSet with a type with non-trivial
>> + // ctors/dtors.
>> + SmallSet<std::string, 2> s1;
>> +
>> + s1.insert("str 1");
>> + s1.insert("str 2");
>> + s1.insert("str 1");
>> +
>> + std::vector<std::string> V(s1.begin(), s1.end());
>> + std::sort(V.begin(), V.end());
>> + EXPECT_EQ(2u, s1.size());
>> + EXPECT_EQ("str 1", V[0]);
>> + EXPECT_EQ("str 2", V[1]);
>> +
>> + s1.insert("str 4");
>> + s1.insert("str 0");
>> + s1.insert("str 4");
>> +
>> + V.assign(s1.begin(), s1.end());
>> + // Make sure the elements are in the expected order.
>> + std::sort(V.begin(), V.end());
>> + EXPECT_EQ(4u, s1.size());
>> + EXPECT_EQ("str 0", V[0]);
>> + EXPECT_EQ("str 1", V[1]);
>> + EXPECT_EQ("str 2", V[2]);
>> + EXPECT_EQ("str 4", V[3]);
>> +}
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> 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/20180712/859da1d5/attachment.html>
More information about the llvm-commits
mailing list