Skip to content

[libc] Implement forward iterators for libc fixed_vector #93916

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 5, 2024
Merged

[libc] Implement forward iterators for libc fixed_vector #93916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0dc44e7
implement libc forward iterators
jameshu15869 May 31, 2024
6995e94
add fixedvector iterator test
jameshu15869 May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libc/src/__support/fixedvector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ template <typename T, size_t CAPACITY> class FixedVector {
return reverse_iterator{&store[item_count]};
}
LIBC_INLINE constexpr reverse_iterator rend() { return store.rend(); }

using iterator = typename cpp::array<T, CAPACITY>::iterator;
LIBC_INLINE constexpr iterator begin() { return store.begin(); }
LIBC_INLINE constexpr iterator end() { return iterator{&store[item_count]}; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does returning store.end() not work?

Suggested change
LIBC_INLINE constexpr iterator end() { return iterator{&store[item_count]}; }
LIBC_INLINE constexpr iterator end() { return store.end(); }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think store.end() ends up using the entire space allocated to the array instead of the actual number of elements inside it. I was getting a memory error with store.end()but I'm not 100% sure that is the cause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense. I'm wondering if something like
return store.begin() + item_count would work better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that account for the size of the objects themselves when doing the pointer arithmetic? i.e. if an int is 4 bytes and you have 4 elements would you want end() to be at store.begin() + 4*4?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iterator type is T * from the template, so it should know how to do pointer arithmetic on it, but it should be caught in your tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I was waiting on rebuilding LLVM this morning but I can try it when I get home later today

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer &store[item_count] and let the compiler do the pointer arithmetic for you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just using the iterator directly makes sure it's the correct iterator type though.

};

} // namespace LIBC_NAMESPACE
Expand Down
10 changes: 10 additions & 0 deletions libc/test/src/__support/fixedvector_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,14 @@ TEST(LlvmLibcFixedVectorTest, Iteration) {
ASSERT_TRUE(++it == v.rend());
for (auto it = v.rbegin(), e = v.rend(); it != e; ++it)
ASSERT_GT(*it, -1);

auto forward_it = v.begin();
ASSERT_EQ(*forward_it, 0);
ASSERT_EQ(*++forward_it, 1);
ASSERT_EQ(*++forward_it, 2);
ASSERT_TRUE(++forward_it == v.end());
for (auto it = v.begin(), e = v.end(); it != e; ++it)
ASSERT_GT(*it, -1);
for (int &x : v)
ASSERT_GE(x, 0);
}
Loading