[libc-commits] [PATCH] D118954: [libc] add a vector internal class

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/CPP/CMakeLists.txt:14
+add_header_library(
+  cpp_with_malloc
+  HDRS
----------------
michaelrj wrote:
> lntue wrote:
> > cpp_vector?
> I named it `cpp_with_malloc` to match `standalone_cpp` but I agree that for the moment `cpp_vector` is a better name.
You can name it just `vector`. The actual target name will be fully qualified - `libc.src.__support.CPP.vector`.


================
Comment at: libc/src/__support/CPP/vector.h:12
+
+#include <limits.h>
+#include <stddef.h> // For size_t.
----------------
Why this required?


================
Comment at: libc/src/__support/CPP/vector.h:32
+  static constexpr size_t GROWTH_FACTOR = 2;
+  static constexpr size_t MAX_SIZE = __SIZE_MAX__;
+
----------------
This can be `~size_t(0)`.


================
Comment at: libc/src/__support/CPP/vector.h:38
+  }
+  constexpr vector<T>(size_t count) : array_size{count} {
+    data_array = static_cast<T *>(malloc(count * sizeof(T)));
----------------
The `std::vector` API requires this constructor to default-insert the elements which in turn requires placement new. So, may be drop this constructor if not required so that we don't deviate with from the `std::vector` behavior.


================
Comment at: libc/src/__support/CPP/vector.h:41
+  }
+  constexpr vector<T>(const vector<T> &other) {
+    array_size = other.capacity();
----------------
Can we keep the copy and move constructors deleted until they are required? Same with copy-assignment and move-assignment operators.


================
Comment at: libc/src/__support/CPP/vector.h:64
+  // can only increase the size of the array
+  constexpr void reserve(size_t new_cap) {
+    if (new_cap < array_size || new_cap > MAX_SIZE)
----------------
Do we have a use-case for this method?


================
Comment at: libc/src/__support/CPP/vector.h:71
+
+  constexpr void shrink_to_fit() {
+    array_size = num_elements;
----------------
Do we have a use-case for this method?


================
Comment at: libc/src/__support/CPP/vector.h:77
+  constexpr void push_back(const T &value) {
+    if (num_elements >= array_size)
+      increase_size(num_elements + 1);
----------------
Will the `>` part of this inequality ever trigger?


================
Comment at: libc/src/__support/CPP/vector.h:85
+    if (pos >= num_elements)
+      num_elements = pos + 1;
+    return data_array[pos];
----------------
I am not sure this is the `std::vector` behavior. There is no bounds checking or implicit `push_back` happening with `std::vector::operator[]`.


================
Comment at: libc/src/__support/CPP/vector.h:94
+  constexpr size_t size() const { return num_elements; }
+  constexpr size_t max_size() const { return MAX_SIZE; }
+
----------------
Why is this method required?


================
Comment at: libc/src/__support/CPP/vector.h:101
+  // items.
+  constexpr void increase_size(size_t new_size) {
+    size_t temp_size = array_size;
----------------
Why is this method public?


================
Comment at: libc/src/__support/CPP/vector.h:106
+    while (temp_size < new_size)
+      temp_size = temp_size * GROWTH_FACTOR;
+    if (temp_size >= MAX_SIZE)
----------------
`GROWTH_FACTOR` is only used here. So, can we replace this line like this;

```
      temp_size <<= 1;
```


================
Comment at: libc/src/__support/CPP/vector.h:107
+      temp_size = temp_size * GROWTH_FACTOR;
+    if (temp_size >= MAX_SIZE)
+      return;
----------------
The `>` should never trigger. Also, this should be an early return. As in, lines 107 and 108 should be moved to the top.


================
Comment at: libc/src/__support/CPP/vector.h:115
+  // calls realloc on data_array so that its size matches array_size
+  constexpr void realloc_array() {
+    data_array = static_cast<T *>(realloc(data_array, array_size * sizeof(T)));
----------------
All methods should be self-contained. So, the new size should be an argument to this method. At that point, this is a one liner and need not be method at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118954/new/

https://reviews.llvm.org/D118954