[libc-commits] [PATCH] D105843: [libc] Add option to run specific tests

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:146
 
-int Test::runTests() {
+int Test::runTests(int argc, char *argv[]) {
   int TestCount = 0;
----------------
aeubanks wrote:
> caitlyncano wrote:
> > aeubanks wrote:
> > > caitlyncano wrote:
> > > > sivachandra wrote:
> > > > > aeubanks wrote:
> > > > > > it's kinda weird to pass argc/argv directly to a function besides `main`. maybe just passing a `std::optional<std::string> Filter` is better
> > > > > I agree - we should move the `argv` parsing code to a separate helper function. It would be cleaner and also scale (we plan to add ability to run flaky tests with run count etc.) @aeubanks' suggestion to use `std::optional` for the argument is perfect.
> > > > So the goal is to make a separate function to generate a Filter string that may/may not exist depending on the argc/argv conditions? I've never used the std::optional template so this is taking me a bit longer than expected to write out, many apologies!
> > > yup
> > > 
> > > if we have argv[1] containing a string, set the `std::optional<std::string>` to `std::optional<std::string>{argv[1]}`
> > > else set the `std::optional<std::string>` to `std::nullopt`
> > Ok sounds great! It looks like std::optional needs another header, is adding #include <optional> in LibcTest.h okay or does libc have our own header somewhere?
> That's a C++ (not a C) header, so `#include <optional>` is fine
Sigh! Even C++ headers cannot be included as they can potentially include other C headers and lead to mixups. We have a standalone, C++ utilities library but it does not have `std::optional`: https://github.com/llvm/llvm-project/tree/main/libc/utils/CPP

One not so C++ like option for the interim is to use `const char *` and condition on whether it is a `nullptr` or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105843