This is an implementation of https://discourse.llvm.org/t/rfc-running-libc-unit-tests-as-integration-tests/69461.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/__support/CPP/string.h | ||
---|---|---|
94 | 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? | |
108 | 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. | |
172 | May be put this in a namespace named internal? | |
libc/test/UnitTest/TestLogger.cpp | ||
6 | 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. | |
libc/test/UnitTest/TestLogger.h | ||
20 | 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. |
libc/src/__support/CPP/string.h | ||
---|---|---|
30 | Forgot to mention: the code style should be snake_case for var names and function names including class/struct members. |
libc/src/__support/CPP/string.h | ||
---|---|---|
94 | I missed this requirement, thx for catching it. | |
108 | 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. | |
libc/test/UnitTest/TestLogger.cpp | ||
6 | 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? | |
libc/test/UnitTest/TestLogger.h | ||
20 | thx for the great suggestion! |
Couple of nits but this is looking great. Thanks for working on this.
libc/src/__support/CPP/string.h | ||
---|---|---|
30 | Nit: NULL_CHARACTER | |
31 | Nit: get_empty_string() | |
108 | 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. | |
libc/test/UnitTest/TestLogger.cpp | ||
6 | 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. |
Forgot to mention: the code style should be snake_case for var names and function names including class/struct members.