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

[Jeff Bailey via Phabricator via libc-commits]

jeffbailey added a comment.

A few minor comments within.  Thanks for doing this.



================
Comment at: libc/src/__support/CPP/StringView.h:22
 // This class will be extended as needed in future.
 class StringView {
 private:
----------------
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. 


================
Comment at: libc/src/__support/CPP/StringView.h:27
 
+  static size_t min(size_t A, size_t B) { return A <= B ? A : B; }
+
----------------
Is it with factoring this out into a separate header and making a template?  It seems likely that min/max will get used in a few places.


================
Comment at: libc/src/__support/CPP/StringView.h:93
 
-    const char *NewStart = Data;
-    size_t PrefixLen = 0;
-    for (; PrefixLen < Len; ++NewStart, ++PrefixLen) {
-      if (*NewStart != C)
-        break;
-    }
+  // An equivalent method is not available in std::string_view.
+  bool equals(StringView Other) const {
----------------
operator== was added in c++17


================
Comment at: libc/src/__support/CPP/StringView.h:100
+  // Check if this string starts with the given Prefix.
+  bool startswith(StringView Prefix) const {
+    return Len >= Prefix.Len &&
----------------
Why not starts_with like in the standard?


================
Comment at: libc/src/__support/CPP/StringView.h:106
+  // Check if this string ends with the given Suffix.
+  bool endswith(StringView Suffix) const {
+    return Len >= Suffix.Len &&
----------------
Why not ends_with like in the standard?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128908