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

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet marked 8 inline comments as done.
gchatelet added inline comments.


================
Comment at: libc/src/__support/CPP/StringView.h:22
 // This class will be extended as needed in future.
 class StringView {
 private:
----------------
courbet wrote:
> 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. 
> 
> 
> 
> 
Since everybody agrees, let's proceed with this patch.

As @courbet said I've created this patch because I couldn't use `iostream` nor `string` in `algorithm_test.cpp` and it was failing in full build mode.
The solution was to disable this particular test in full build mode.

@sivachandra AFAIU full build is really here to make sure that we can build `llvmlibc.a` while using llvm libc's header and library. To me it serves as an integration test. It would make sense to run functional tests in full build more (i.e. making sure that the C function behaves like they should) but I don't think running unittests brings anything.

Let's discuss this in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128908