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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Mostly LGTM with some comments inline. Can you update your other patch which uses this so that we can understand what exactly is required and why. Then we can actually compare with other possibilities may be.



================
Comment at: libc/src/__support/CPP/CMakeLists.txt:14
+add_header_library(
+  cpp_with_malloc
+  HDRS
----------------
michaelrj wrote:
> 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.
The name `standalone_cpp` predates fully qualified target names in libc. I agree that it should be split out into separate targets.


================
Comment at: libc/src/__support/CPP/vector.h:91
+      temp_size = temp_size * GROWTH_FACTOR;
+    if (temp_size >= MAX_SIZE)
+      return;
----------------
AFAICT, there is no way to update `MAX_SIZE` and it is set to `~size_t(0)`. Which means the the `>` will never trigger with any number. Also, you need to have some sort of an early return here because the growth can wrap the `temp_size` value around.


================
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)));
----------------
michaelrj wrote:
> 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.
Not having functionality is fine, but deviating from the `std::vector` behavior will make it confusing. Also, this particular constructor is allocating capacity as opposed to the `std::vector` behavior of inserting default constructed elements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118954