Skip to content

[libc][FixedVector] Add more helper methods #94278

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

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

PiJoules
Copy link
Contributor

@PiJoules PiJoules commented Jun 3, 2024

This adds:

  • A ctor accepting a start and end iterator
  • A ctor accepting a count and const T&
  • size()
  • subscript operators
  • begin() and end() iterators

@llvmbot
Copy link
Member

llvmbot commented Jun 3, 2024

@llvm/pr-subscribers-libc

Author: None (PiJoules)

Changes

This adds:

  • A ctor accepting a start and end iterator
  • A ctor accepting a count and const T&
  • size()
  • subscript operators
  • begin() and end() iterators

Full diff: https://github.com/llvm/llvm-project/pull/94278.diff

3 Files Affected:

  • (modified) libc/src/__support/fixedvector.h (+20)
  • (modified) libc/test/src/__support/CMakeLists.txt (+1)
  • (modified) libc/test/src/__support/fixedvector_test.cpp (+35)
diff --git a/libc/src/__support/fixedvector.h b/libc/src/__support/fixedvector.h
index 81747ee10067c..b06321887e7a1 100644
--- a/libc/src/__support/fixedvector.h
+++ b/libc/src/__support/fixedvector.h
@@ -24,6 +24,16 @@ template <typename T, size_t CAPACITY> class FixedVector {
 public:
   constexpr FixedVector() = default;
 
+  template <typename It> constexpr FixedVector(It begin, It end) {
+    for (; begin != end; ++begin)
+      push_back(*begin);
+  }
+
+  constexpr FixedVector(size_t count, const T &value) {
+    for (size_t i = 0; i < count; ++i)
+      push_back(value);
+  }
+
   bool push_back(const T &obj) {
     if (item_count == CAPACITY)
       return false;
@@ -43,8 +53,14 @@ template <typename T, size_t CAPACITY> class FixedVector {
     return true;
   }
 
+  T &operator[](size_t idx) { return store[idx]; }
+
+  const T &operator[](size_t idx) const { return store[idx]; }
+
   bool empty() const { return item_count == 0; }
 
+  size_t size() const { return item_count; }
+
   // Empties the store for all practical purposes.
   void reset() { item_count = 0; }
 
@@ -63,6 +79,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]}; }
 };
 
 } // namespace LIBC_NAMESPACE
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 663aa2bb82cae..936cfe4e2a20c 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -132,6 +132,7 @@ add_libc_test(
   SRCS
     fixedvector_test.cpp
   DEPENDS
+    libc.src.__support.CPP.array
     libc.src.__support.fixedvector
 )
 
diff --git a/libc/test/src/__support/fixedvector_test.cpp b/libc/test/src/__support/fixedvector_test.cpp
index 4e92081321de7..c61b9ca815643 100644
--- a/libc/test/src/__support/fixedvector_test.cpp
+++ b/libc/test/src/__support/fixedvector_test.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "src/__support/CPP/array.h"
 #include "src/__support/fixedvector.h"
 #include "test/UnitTest/Test.h"
 
@@ -59,3 +60,37 @@ TEST(LlvmLibcFixedVectorTest, Iteration) {
   for (auto it = v.rbegin(), e = v.rend(); it != e; ++it)
     ASSERT_GT(*it, -1);
 }
+
+TEST(LlvmLibcFixedVectorTest, ConstructionFromIterators) {
+  LIBC_NAMESPACE::cpp::array<int, 4> arr{1, 2, 3, 4};
+  LIBC_NAMESPACE::FixedVector<int, 5> vec(arr.begin(), arr.end());
+  ASSERT_EQ(vec.size(), arr.size());
+  for (size_t i = 0; i < arr.size(); ++i)
+    ASSERT_EQ(vec[i], arr[i]);
+}
+
+TEST(LlvmLibcFixedVectorTest, ConstructionFromCountAndValue) {
+  constexpr int kVal = 10;
+  // TODO: If the first argument here were just `4`, then we'd have no way to
+  // disambiguate between the FixedVector ctor that uses iterators vs the one
+  // taking a count and `cosnt T &`. Using `4` would result in a compile error.
+  // Formally, we can ensure the count + reference ctor is used if we gate the
+  // iterator ctor on checking if the type has the `input_iterator_tag` via
+  // iterator_traits, but we'd have to plumb that through which can be done
+  // separately. Note the snafu we hit here only happens because we happen to
+  // test with containters using integral types.
+  LIBC_NAMESPACE::FixedVector<int, 5> vec(size_t(4), kVal);
+  ASSERT_EQ(vec.size(), size_t(4));
+  for (size_t i = 0; i < vec.size(); ++i)
+    ASSERT_EQ(vec[i], kVal);
+}
+
+TEST(LlvmLibcFixedVectorTest, ForwardIteration) {
+  LIBC_NAMESPACE::cpp::array<int, 4> arr{1, 2, 3, 4};
+  LIBC_NAMESPACE::FixedVector<int, 5> vec(arr.begin(), arr.end());
+  ASSERT_EQ(vec.size(), arr.size());
+  for (auto it = vec.begin(); it != vec.end(); ++it) {
+    auto idx = it - vec.begin();
+    ASSERT_EQ(*it, arr[idx]);
+  }
+}

@lntue lntue requested a review from jhuber6 June 4, 2024 15:19
@@ -24,6 +24,16 @@ template <typename T, size_t CAPACITY> class FixedVector {
public:
constexpr FixedVector() = default;

template <typename It> constexpr FixedVector(It begin, It end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this constructor template needs to be a bit more restrictive on the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restricted it to only cpp::array input iterators.

This adds:
- A ctor accepting a start and end iterator
- A ctor accepting a count and const T&
- size()
- subscript operators
- begin() and end() iterators
@PiJoules PiJoules force-pushed the add-more-methods-to-fixed-vector branch from cb8699f to 0aedcbb Compare June 5, 2024 21:34
@PiJoules PiJoules requested a review from lntue June 5, 2024 21:34
@PiJoules PiJoules merged commit 649edb8 into llvm:main Jun 6, 2024
6 checks passed
@PiJoules PiJoules deleted the add-more-methods-to-fixed-vector branch June 6, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants