[llvm] r298440 - Revert "Improve StringMap iterator support."
Zachary Turner via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 21 15:57:27 PDT 2017
Thanks for the heads up, I plan to take a look at this in a few minutes and
hopefully resubmit.
On Tue, Mar 21, 2017 at 3:08 PM Richard Smith <richard at metafoo.co.uk> wrote:
> On 21 March 2017 at 14:23, Zachary Turner via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: zturner
> Date: Tue Mar 21 16:23:57 2017
> New Revision: 298440
>
> URL: http://llvm.org/viewvc/llvm-project?rev=298440&view=rev
> Log:
> Revert "Improve StringMap iterator support."
>
> This is causing crashes in clang, so reverting until the problem
> is figured out.
>
> Modified:
> llvm/trunk/include/llvm/ADT/STLExtras.h
> llvm/trunk/include/llvm/ADT/StringMap.h
> llvm/trunk/unittests/ADT/StringMapTest.cpp
>
> Modified: llvm/trunk/include/llvm/ADT/STLExtras.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/STLExtras.h?rev=298440&r1=298439&r2=298440&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/STLExtras.h (original)
> +++ llvm/trunk/include/llvm/ADT/STLExtras.h Tue Mar 21 16:23:57 2017
> @@ -893,8 +893,7 @@ auto partition(R &&Range, UnaryPredicate
> /// SmallVector with elements of the vector. This is useful, for example,
> /// when you want to iterate a range and then sort the results.
> template <unsigned Size, typename R>
> -SmallVector<typename std::remove_const<detail::ValueOfRange<R>>::type,
> Size>
> -to_vector(R &&Range) {
> +SmallVector<detail::ValueOfRange<R>, Size> to_vector(R &&Range) {
> return {std::begin(Range), std::end(Range)};
> }
>
>
> Modified: llvm/trunk/include/llvm/ADT/StringMap.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=298440&r1=298439&r2=298440&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
> +++ llvm/trunk/include/llvm/ADT/StringMap.h Tue Mar 21 16:23:57 2017
> @@ -15,13 +15,13 @@
> #define LLVM_ADT_STRINGMAP_H
>
> #include "llvm/ADT/StringRef.h"
> -#include "llvm/ADT/iterator.h"
> #include "llvm/Support/Allocator.h"
> #include "llvm/Support/PointerLikeTypeTraits.h"
> #include <cassert>
> #include <cstdint>
> #include <cstdlib>
> #include <cstring>
> +#include <utility>
> #include <initializer_list>
> #include <new>
> #include <utility>
> @@ -32,7 +32,6 @@ namespace llvm {
> class StringMapConstIterator;
> template<typename ValueT>
> class StringMapIterator;
> - template <typename ValueT> class StringMapKeyIterator;
> template<typename ValueTy>
> class StringMapEntry;
>
> @@ -313,11 +312,6 @@ public:
> return const_iterator(TheTable+NumBuckets, true);
> }
>
> - llvm::iterator_range<StringMapKeyIterator<ValueTy>> keys() const {
> - return make_range(StringMapKeyIterator<ValueTy>(begin()),
> - StringMapKeyIterator<ValueTy>(end()));
> - }
> -
> iterator find(StringRef Key) {
> int Bucket = FindKey(Key);
> if (Bucket == -1) return end();
> @@ -450,39 +444,42 @@ public:
> }
> };
>
> -template <typename DerivedTy, typename ValueTy>
> -class StringMapIterBase
> - : public iterator_facade_base<DerivedTy, std::forward_iterator_tag,
> - ValueTy> {
> +template <typename ValueTy> class StringMapConstIterator {
> protected:
> StringMapEntryBase **Ptr = nullptr;
>
> public:
> - StringMapIterBase() = default;
> + typedef StringMapEntry<ValueTy> value_type;
>
> - explicit StringMapIterBase(StringMapEntryBase **Bucket,
> - bool NoAdvance = false)
> - : Ptr(Bucket) {
> + StringMapConstIterator() = default;
> +
> + explicit StringMapConstIterator(StringMapEntryBase **Bucket,
> + bool NoAdvance = false)
> + : Ptr(Bucket) {
> if (!NoAdvance) AdvancePastEmptyBuckets();
> }
>
> - DerivedTy &operator=(const DerivedTy &Other) {
> - Ptr = Other.Ptr;
> - return static_cast<DerivedTy &>(*this);
> + const value_type &operator*() const {
> + return *static_cast<StringMapEntry<ValueTy>*>(*Ptr);
> + }
> + const value_type *operator->() const {
> + return static_cast<StringMapEntry<ValueTy>*>(*Ptr);
> }
>
> - bool operator==(const DerivedTy &RHS) const { return Ptr == RHS.Ptr; }
> + bool operator==(const StringMapConstIterator &RHS) const {
> + return Ptr == RHS.Ptr;
> + }
> + bool operator!=(const StringMapConstIterator &RHS) const {
> + return Ptr != RHS.Ptr;
> + }
>
> - DerivedTy &operator++() { // Preincrement
> + inline StringMapConstIterator& operator++() { // Preincrement
> ++Ptr;
> AdvancePastEmptyBuckets();
> - return static_cast<DerivedTy &>(*this);
> + return *this;
> }
> -
> - DerivedTy operator++(int) { // Post-increment
> - DerivedTy Tmp(Ptr);
> - ++*this;
> - return Tmp;
> + StringMapConstIterator operator++(int) { // Postincrement
> + StringMapConstIterator tmp = *this; ++*this; return tmp;
> }
>
> private:
> @@ -492,67 +489,22 @@ private:
> }
> };
>
> -template <typename ValueTy>
> -class StringMapConstIterator
> - : public StringMapIterBase<StringMapConstIterator<ValueTy>,
> - const StringMapEntry<ValueTy>> {
> - using base = StringMapIterBase<StringMapConstIterator<ValueTy>,
> - const StringMapEntry<ValueTy>>;
> -
> -public:
> - StringMapConstIterator() = default;
> - explicit StringMapConstIterator(StringMapEntryBase **Bucket,
> - bool NoAdvance = false)
> - : base(Bucket, NoAdvance) {}
> -
> - const StringMapEntry<ValueTy> &operator*() const {
> - return *static_cast<const StringMapEntry<ValueTy> *>(*this->Ptr);
> - }
> -};
> -
> -template <typename ValueTy>
> -class StringMapIterator : public
> StringMapIterBase<StringMapIterator<ValueTy>,
> -
> StringMapEntry<ValueTy>> {
> - using base =
> - StringMapIterBase<StringMapIterator<ValueTy>,
> StringMapEntry<ValueTy>>;
> -
> +template<typename ValueTy>
> +class StringMapIterator : public StringMapConstIterator<ValueTy> {
> public:
> StringMapIterator() = default;
> +
> explicit StringMapIterator(StringMapEntryBase **Bucket,
> bool NoAdvance = false)
> - : base(Bucket, NoAdvance) {}
> -
> - StringMapEntry<ValueTy> &operator*() const {
> - return *static_cast<StringMapEntry<ValueTy> *>(*this->Ptr);
> + : StringMapConstIterator<ValueTy>(Bucket, NoAdvance) {
> }
>
> - operator StringMapConstIterator<ValueTy>() const {
> - return StringMapConstIterator<ValueTy>(this->Ptr, false);
>
>
> The problem is that this conversion tries to "advance" the iterator, which
> doesn't work if it's the end iterator. You should probably pass 'true' here.
>
>
> + StringMapEntry<ValueTy> &operator*() const {
> + return *static_cast<StringMapEntry<ValueTy>*>(*this->Ptr);
> }
> -};
> -
> -template <typename ValueTy>
> -class StringMapKeyIterator
> - : public iterator_adaptor_base<StringMapKeyIterator<ValueTy>,
> - StringMapConstIterator<ValueTy>,
> - std::forward_iterator_tag, StringRef> {
> - using base = iterator_adaptor_base<StringMapKeyIterator<ValueTy>,
> - StringMapConstIterator<ValueTy>,
> - std::forward_iterator_tag,
> StringRef>;
> -
> -public:
> - StringMapKeyIterator() = default;
> -
> - explicit StringMapKeyIterator(StringMapConstIterator<ValueTy> Iter)
> - : base(std::move(Iter)) {}
> -
> - StringRef &operator*() {
> - Key = this->wrapped()->getKey();
> - return Key;
> + StringMapEntry<ValueTy> *operator->() const {
> + return static_cast<StringMapEntry<ValueTy>*>(*this->Ptr);
> }
> -
> -private:
> - StringRef Key;
> };
>
> } // end namespace llvm
>
> Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=298440&r1=298439&r2=298440&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/StringMapTest.cpp Tue Mar 21 16:23:57 2017
> @@ -8,7 +8,6 @@
>
> //===----------------------------------------------------------------------===//
>
> #include "llvm/ADT/StringMap.h"
> -#include "llvm/ADT/StringSet.h"
> #include "llvm/ADT/Twine.h"
> #include "llvm/Support/DataTypes.h"
> #include "gtest/gtest.h"
> @@ -270,34 +269,6 @@ TEST_F(StringMapTest, InsertRehashingPai
> EXPECT_EQ(42u, It->second);
> }
>
> -TEST_F(StringMapTest, IterMapKeys) {
> - StringMap<int> Map;
> - Map["A"] = 1;
> - Map["B"] = 2;
> - Map["C"] = 3;
> - Map["D"] = 3;
> -
> - auto Keys = to_vector<4>(Map.keys());
> - std::sort(Keys.begin(), Keys.end());
> -
> - SmallVector<StringRef, 4> Expected = {"A", "B", "C", "D"};
> - EXPECT_EQ(Expected, Keys);
> -}
> -
> -TEST_F(StringMapTest, IterSetKeys) {
> - StringSet<> Set;
> - Set.insert("A");
> - Set.insert("B");
> - Set.insert("C");
> - Set.insert("D");
> -
> - auto Keys = to_vector<4>(Set.keys());
> - std::sort(Keys.begin(), Keys.end());
> -
> - SmallVector<StringRef, 4> Expected = {"A", "B", "C", "D"};
> - EXPECT_EQ(Expected, Keys);
> -}
> -
> // Create a non-default constructable value
> struct StringMapTestStruct {
> StringMapTestStruct(int i) : i(i) {}
>
>
> _______________________________________________
> 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/20170321/c0ce85ab/attachment-0001.html>
More information about the llvm-commits
mailing list