[libc-commits] [PATCH] D147231: [libc] Adds string and TestLogger classes, use them in LibcTest

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

Couple of nits but this is looking great. Thanks for working on this.



================
Comment at: libc/src/__support/CPP/string.h:29
+private:
+  static constexpr char kNullCharacter = '\0';
+  static constexpr char *getEmptyString() {
----------------
Nit: `NULL_CHARACTER`



================
Comment at: libc/src/__support/CPP/string.h:30
+  static constexpr char kNullCharacter = '\0';
+  static constexpr char *getEmptyString() {
+    return const_cast<char *>(&kNullCharacter);
----------------
Nit: `get_empty_string()`


================
Comment at: libc/src/__support/CPP/string.h:107
+    char *OldBuffer = Buffer;
+    Buffer = static_cast<char *>(::malloc(NewCapacity));
+    Capacity = NewCapacity;
----------------
gchatelet wrote:
> sivachandra wrote:
> > Is this a good candidate for a `realloc`? If not, then we should follow this pattern for allocations in general in this file: https://libc.llvm.org/dev/code_style.html#allocations-in-the-libc-runtime-code.
> It is indeed a good use of `realloc`. FWIW I initially tried to use the suggested pattern but had a bunch of linker errors complaining about missing `__llvm_libc_delete`.
> As a matter of fact I couldn't find the symbol in the codebase.
> 
The standard does not allow us to implement `delete` functions inline. So, they are implemented in `new.cpp`. Given the linker error, I suspect you missed adding a dep on `lib.src.__support.CPP.new`. I guess its not required now anyway.


================
Comment at: libc/test/UnitTest/TestLogger.cpp:5
+
+#include <stdio.h> // fwrite, fputs, stdout
+
----------------
gchatelet wrote:
> sivachandra wrote:
> > We want this to work on really minimal systems on which `fwrite`, `fputs` `stdout` etc are not [yet] available. So, we should use a simple function like https://github.com/llvm/llvm-project/blob/main/libc/src/__support/OSUtil/linux/io.h which does a raw write to stdout.
> It would be helpful to provide a version of `__llvm_libc::write_to_stderr` that takes the length as an argument. Would that be OK?
> As is, the call is suboptimal for single `char` and `string_view`.
A lot of those utils are improved as required. So, if you think this patch will benefit, either add more args or add overloads. Totally fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147231