[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:
> > 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.
I'm adding a TODO, and I will address this in a future patch.


================
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:
> 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.
ah, I see. I understand what you mean now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118954