-
Notifications
You must be signed in to change notification settings - Fork 868
inplace_iterator: Add enough iteration to support std::erase_if #8877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,11 @@ | |
| #ifndef wasm_support_inplace_vector_h | ||
| #define wasm_support_inplace_vector_h | ||
|
|
||
| #include <algorithm> | ||
| #include <array> | ||
| #include <cassert> | ||
| #include <iterator> | ||
| #include <type_traits> | ||
| #include <vector> | ||
|
|
||
| #include "support/parent_index_iterator.h" | ||
|
|
@@ -134,9 +137,9 @@ template<typename T, size_t N> class inplace_vector { | |
| Iterator(inplace_vector<T, N>* parent, size_t index) | ||
| : wasm::ParentIndexIterator<inplace_vector<T, N>*, Iterator>{parent, | ||
| index} {} | ||
| Iterator(const Iterator& other) = default; | ||
|
|
||
| T& operator*() { return (*this->parent)[this->index]; } | ||
| T* operator->() { return &(*this->parent)[this->index]; } | ||
| }; | ||
|
|
||
| struct ConstIterator | ||
|
|
@@ -151,20 +154,70 @@ template<typename T, size_t N> class inplace_vector { | |
| ConstIterator(const ConstIterator& other) = default; | ||
|
|
||
| const T& operator*() const { return (*this->parent)[this->index]; } | ||
| const T* operator->() const { return &(*this->parent)[this->index]; } | ||
| }; | ||
|
|
||
| Iterator begin() { return Iterator(this, 0); } | ||
| Iterator end() { return Iterator(this, size()); } | ||
| ConstIterator begin() const { return ConstIterator(this, 0); } | ||
| ConstIterator end() const { return ConstIterator(this, size()); } | ||
|
|
||
| void erase(Iterator a, Iterator b) { | ||
| // Atm we only support erasing at the end, which is very efficient. | ||
| assert(b == end()); | ||
| resize(a.index); | ||
| Iterator erase(ConstIterator first, ConstIterator last) { | ||
| assert(first.index <= last.index); | ||
| assert(last.index <= usedFixed); | ||
| size_t numToErase = last.index - first.index; | ||
| if (numToErase > 0) { | ||
| std::move(fixed.begin() + last.index, | ||
| fixed.begin() + usedFixed, | ||
| fixed.begin() + first.index); | ||
| usedFixed -= numToErase; | ||
| } | ||
| return Iterator(this, first.index); | ||
| } | ||
|
|
||
| Iterator erase(Iterator first, Iterator last) { | ||
| return erase(ConstIterator(this, first.index), | ||
| ConstIterator(this, last.index)); | ||
| } | ||
|
|
||
| Iterator erase(ConstIterator pos) { return erase(pos, pos + 1); } | ||
|
|
||
| Iterator erase(Iterator pos) { return erase(pos, pos + 1); } | ||
| }; | ||
|
|
||
| namespace detail { | ||
|
|
||
| template<typename T> struct is_inplace_vector_or_derived { | ||
| private: | ||
| template<typename U, size_t N> | ||
| static std::true_type test(const inplace_vector<U, N>*); | ||
| static std::false_type test(...); | ||
|
|
||
| public: | ||
| static constexpr bool value = decltype(test(std::declval<T*>()))::value; | ||
| }; | ||
|
|
||
| } // namespace detail | ||
|
|
||
| template<typename Vector, typename Pred> | ||
| requires detail::is_inplace_vector_or_derived<Vector>::value | ||
| size_t erase_if(Vector& c, Pred pred) { | ||
| auto it = std::remove_if(c.begin(), c.end(), pred); | ||
| auto r = std::distance(it, c.end()); | ||
| c.erase(it, c.end()); | ||
| return r; | ||
| } | ||
|
|
||
| } // namespace wasm | ||
|
|
||
| namespace std { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kind of weird that you have to provide a new overload of a std function to support custom containers, but it looks like this is the standard practice?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't know how else to do this. The compiler errors without these overloads. |
||
|
|
||
| template<typename Vector, typename Pred> | ||
| requires wasm::detail::is_inplace_vector_or_derived<Vector>::value | ||
| size_t erase_if(Vector& c, Pred pred) { | ||
| return wasm::erase_if(c, pred); | ||
| } | ||
|
|
||
| } // namespace std | ||
|
|
||
| #endif // wasm_support_inplace_vector_h | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're still hiding use of
requiresbehind ifdefs so jukka's old system compilers still work... but if we're not testing that on CI, maybe we shouldn't worry about that beyond letting him fix things up as necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see the lattice code has such ifdefs, but we have other
requireswithout? E.g.binaryen/src/wasm-type-printing.h
Line 119 in 82b8645
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Landing, if there is an issue later I can fix it up.