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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/CPP/string.h:93
+
+  const char *c_str() {
+    if (empty() || back() != '\0') {
----------------
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?


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


================
Comment at: libc/src/__support/CPP/string.h:171
+
+template <typename T> string to_dec_string(T Value) {
+  char DecBuf[IntegerToString::dec_bufsize<T>()];
----------------
May be put this in a namespace named `internal`?


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


================
Comment at: libc/test/UnitTest/TestLogger.h:19
+public:
+  TestLogger();
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147231