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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/CPP/CMakeLists.txt:14
+add_header_library(
+  cpp_with_malloc
+  HDRS
----------------
sivachandra wrote:
> 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`.
that's fine, but in that case the `standalone_cpp` header library should probably be renamed to just `standalone`, or possibly split out into its component headers for consistency.


================
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)));
----------------
sivachandra wrote:
> 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.
While not required, this constructor is used to make an array of size 0 to save memory on an array that may never be written to. In addition, this class is already not capable of all of the things expected of `std::vector`, due to not being able to construct or destruct classes, so I think it's fine.


================
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)
----------------
sivachandra wrote:
> Do we have a use-case for this method?
not yet, I figured it and `shrink_to_fit` would be useful for the future. They would mostly come up in a situation where there is memory sensitivity (we don't want to store more than necessary) but a known maximum size of the array. In that case one could reserve that maximum size, write to the array, then shrink to fit. This could theoretically be useful for some printf conversions (e.g. integers have a maximum number of resulting digits, but the actual number will likely be smaller) although for those use cases it's likely not worth the time to shrink, since the memory is negligible.

Since I don't have a current use case, I've removed them.


================
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);
----------------
sivachandra wrote:
> Will the `>` part of this inequality ever trigger?
with the current setup, no, but having it as an inequality will protect against future off-by-one errors.


================
Comment at: libc/src/__support/CPP/vector.h:85
+    if (pos >= num_elements)
+      num_elements = pos + 1;
+    return data_array[pos];
----------------
sivachandra wrote:
> I am not sure this is the `std::vector` behavior. There is no bounds checking or implicit `push_back` happening with `std::vector::operator[]`.
I added that to keep track of `num_elements`, given that the function intended to do that (`resize`) requires use of creating new objects when resizing. I've switched back to using that function and moved `increase_size()` back into being a private function, but something here has to be able to increase the size of the array.


================
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; }
+
----------------
sivachandra wrote:
> Why is this method required?
for bounds checking in client applications. Since this class can't use exceptions, attempting to allocate over the maximum size is undefined behavior, and so checking that is on the client.


================
Comment at: libc/src/__support/CPP/vector.h:101
+  // items.
+  constexpr void increase_size(size_t new_size) {
+    size_t temp_size = array_size;
----------------
sivachandra wrote:
> Why is this method public?
see the response on `operator[]`, but it was public so that the array could be resized even when not using push back (specifically in the `RandomWriteOrderedReadTest`, which matches one of the real situations for printf). I've made it private again, and readded the resize function to effectively handle the same situation.


================
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)
----------------
sivachandra wrote:
> `GROWTH_FACTOR` is only used here. So, can we replace this line like this;
> 
> ```
>       temp_size <<= 1;
> ```
I added growth factor as a constexpr variable to avoid magic constants. While that would be logically equivalent, I think that multiplying by the growth factor is clearer.


================
Comment at: libc/src/__support/CPP/vector.h:107
+      temp_size = temp_size * GROWTH_FACTOR;
+    if (temp_size >= MAX_SIZE)
+      return;
----------------
sivachandra wrote:
> The `>` should never trigger. Also, this should be an early return. As in, lines 107 and 108 should be moved to the top.
not necessarily. Since `temp_size` is >= `new_size`, it may be the case that `new_size` is less than `max_size`, but `array_size * 2` is greater than `max_size`. For example if `max_size` was 10, and `array_size` was 8, then calling `increase_size(9)` would cause `array_size` to be doubled to 16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118954