[libc-commits] [PATCH] D128908: [libc][utils] Add more methods to StringView

[Clement Courbet via Phabricator via libc-commits]

courbet added inline comments.


================
Comment at: libc/src/__support/CPP/StringView.h:22
 // This class will be extended as needed in future.
 class StringView {
 private:
----------------
jeffbailey wrote:
> sivachandra wrote:
> > courbet wrote:
> > > @sivachandra Do we really want to keep duplicating the whole libc++ in there ? I can see why we can't use gtest to test the libc, but we should be able to use gtest to test individual internal components. Then we would not need to reimplement the world.
> > > 
> > > internal unit tests would test `__llvm_libc::CodeUnderTest` using the system libc and `gtest`
> > > libc API tests would use the extern C `CodeUnderTest` build with `-ffrestanding` and using `llvm-libc` and `LibCTest`
> > > 
> > > Does that make sense ? At least for code that doe snot rely on `errno` and such weird things.
> > > @sivachandra Do we really want to keep duplicating the whole libc++ in there ?
> > 
> > Just pointing out, a large part of that duplicated code is used in the actual libc runtime code. So, while it might seem like unnecessarily duplicated especially when it seems like it is being used only for testing, it is not going to waste.
> > 
> > > I can see why we can't use gtest to test the libc, but we should be able to use gtest to test individual internal components. Then we would not need to reimplement the world.
> > > 
> > > internal unit tests would test `__llvm_libc::CodeUnderTest` using the system libc and `gtest`
> > > libc API tests would use the extern C `CodeUnderTest` build with `-ffrestanding` and using `llvm-libc` and `LibCTest`
> > 
> > We almost never test the C symbols in our unittests. All of our unittests test members in the `__llvm_libc` namespace. So, if we go with your suggestion, then we can stick with gtest almost always. But, we have moved away from using gtest because we build and run our unittests in two different modes:
> > 
> >     1. The default mode in which we use public headers from the system libc. Of course, only those functions which do not depend on the definitions in the header files can be tested in this manner. For example, most of the functions in `string.h` and `math.h`. The unittests for those functions can very well use gtest, include the C++ standard library etc and it should all just work fine.
> >     2. The //full build// mode in which we test the libc against its own public headers. This mode prevents us from including the C++ standard library headers, gtest headers or any other third party library headers as they can themselves potentially include the libc headers. 
> > 
> > > Does that make sense ? At least for code that doe snot rely on `errno` and such weird things.
> > 
> > Beyond `errno`, not so weird things like macro definitions and type definitions can also cause problems in the full build mode.
> I wonder if the conversation about gtest could be forked off to a different thread, as I'd really like this patch in for the TZ parsing stuff and it seem a bit orthogonal to the cpp::StringView improvements. 
I'm not opposed to this particular patch to be clear, please proceed :)


================
Comment at: libc/src/__support/CPP/StringView.h:22
 // This class will be extended as needed in future.
 class StringView {
 private:
----------------
courbet wrote:
> jeffbailey wrote:
> > sivachandra wrote:
> > > courbet wrote:
> > > > @sivachandra Do we really want to keep duplicating the whole libc++ in there ? I can see why we can't use gtest to test the libc, but we should be able to use gtest to test individual internal components. Then we would not need to reimplement the world.
> > > > 
> > > > internal unit tests would test `__llvm_libc::CodeUnderTest` using the system libc and `gtest`
> > > > libc API tests would use the extern C `CodeUnderTest` build with `-ffrestanding` and using `llvm-libc` and `LibCTest`
> > > > 
> > > > Does that make sense ? At least for code that doe snot rely on `errno` and such weird things.
> > > > @sivachandra Do we really want to keep duplicating the whole libc++ in there ?
> > > 
> > > Just pointing out, a large part of that duplicated code is used in the actual libc runtime code. So, while it might seem like unnecessarily duplicated especially when it seems like it is being used only for testing, it is not going to waste.
> > > 
> > > > I can see why we can't use gtest to test the libc, but we should be able to use gtest to test individual internal components. Then we would not need to reimplement the world.
> > > > 
> > > > internal unit tests would test `__llvm_libc::CodeUnderTest` using the system libc and `gtest`
> > > > libc API tests would use the extern C `CodeUnderTest` build with `-ffrestanding` and using `llvm-libc` and `LibCTest`
> > > 
> > > We almost never test the C symbols in our unittests. All of our unittests test members in the `__llvm_libc` namespace. So, if we go with your suggestion, then we can stick with gtest almost always. But, we have moved away from using gtest because we build and run our unittests in two different modes:
> > > 
> > >     1. The default mode in which we use public headers from the system libc. Of course, only those functions which do not depend on the definitions in the header files can be tested in this manner. For example, most of the functions in `string.h` and `math.h`. The unittests for those functions can very well use gtest, include the C++ standard library etc and it should all just work fine.
> > >     2. The //full build// mode in which we test the libc against its own public headers. This mode prevents us from including the C++ standard library headers, gtest headers or any other third party library headers as they can themselves potentially include the libc headers. 
> > > 
> > > > Does that make sense ? At least for code that doe snot rely on `errno` and such weird things.
> > > 
> > > Beyond `errno`, not so weird things like macro definitions and type definitions can also cause problems in the full build mode.
> > I wonder if the conversation about gtest could be forked off to a different thread, as I'd really like this patch in for the TZ parsing stuff and it seem a bit orthogonal to the cpp::StringView improvements. 
> I'm not opposed to this particular patch to be clear, please proceed :)
> Just pointing out, a large part of that duplicated code is used in the actual libc runtime code.

For code were that's the case, then that's fine. But I've seen Guillaume suffer from not being able to use gtest (and he implemented this patch for tests if I'm not mistaken).

> We almost never test the C symbols in our unittests. All of our unittests test members in the __llvm_libc namespace. So, if we go with your suggestion, then we can stick with gtest almost always. 

I think this is great.

> The full build mode in which we test the libc against its own public headers. This mode prevents us from including the C++ standard library headers, gtest headers or any other third party library headers as they can themselves potentially include the libc headers.

My suggestion would be to have some tests not be run in full build mode, and therefore let these use gtest and all c++ features directly. For example, the tests for the memcpy `SizedOp` really focus on testing the c++ logic there. I think it't fine to assume that the underlying libc is working, and therefore to use whatever is installed on the system. 






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128908