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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: libc/src/__support/CPP/string.h:93
+
+  const char *c_str() {
+    if (empty() || back() != '\0') {
----------------
sivachandra wrote:
> C++11 and on wards, it is expected that `c_str` and `data` are equivalent. Making this class exhibit older behavior can be confusing. Any reason why you have chosen to implement the old behavior?
I missed this requirement, thx for catching it.
I've made sure that data() and c_str() always point to the same pointer and that the string is always null terminated.


================
Comment at: libc/src/__support/CPP/string.h:107
+    char *OldBuffer = Buffer;
+    Buffer = static_cast<char *>(::malloc(NewCapacity));
+    Capacity = NewCapacity;
----------------
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.



================
Comment at: libc/test/UnitTest/TestLogger.cpp:5
+
+#include <stdio.h> // fwrite, fputs, stdout
+
----------------
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`.


================
Comment at: libc/test/UnitTest/TestLogger.h:19
+public:
+  TestLogger();
+
----------------
sivachandra wrote:
> If you can make this `constexpr`, something like this:
> 
> ```
>   constexpr TestLogger() = default;
> ```
> 
> then, we don't need the function `log()` below. We can simply do:
> 
> ```
> extern TestLogger tlog; // Bikeshed the name here but I find the name "log" to be very generic.
> ```
> 
> Irrespective of whether we want to use a function returning a reference to a file static object or an extern non-static-global, the constructor should be made `constexpr`. The reason is that, global objects with a non-constexpr constructor will require runtime initialization. So, we will not be able to run tests on platforms which do not [yet] support global object initialization.
thx for the great suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147231