[libc-commits] [PATCH] D91399: Add strncpy implementation.

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added a comment.

Thx for the patch!



================
Comment at: libc/src/string/strncpy.cpp:19
+                                    const char *__restrict src, size_t n) {
+  for (size_t i = 0, j = 0; i < n; ++i) {
+    if (*(src + j) != '\0') {
----------------
sivachandra wrote:
> cheng.w wrote:
> > sivachandra wrote:
> > > WDYT about a `memcpy` followed by a `memset` (for the null characters)? Both these functions in libcs are //optimized// in some manner so we will be leveraging that. We will need an additional `strlen` operation though.
> > By referring to `strcpy`, I used `memcpy` and `strlen` at first. 
> > But it seems that this method avoids the overhead of:
> > - `strlen` goes through `src` for the length.
> > - function calls.
> > I am not sure whick method is more appropriate. So I upload it for suggestions.
> > By referring to `strcpy`, I used `memcpy` and `strlen` at first. 
> > But it seems that this method avoids the overhead of:
> > - `strlen` goes through `src` for the length.
> > - function calls.
> > I am not sure whick method is more appropriate. So I upload it for suggestions.
> 
> I am not opposed to any approach. At the same time, I do not feel qualified enough to drive an opinion. So, adding @gchatelet for guidance.
It's hard to be assertive on which one is best here. It certainly depends on how `strncpy` is used (buffer sizes, value of `n`, size of the zeroing, ...).
I'd say let's keep the implementation simple and optimize if we see performance issues.





================
Comment at: libc/src/string/strncpy.cpp:22
+      *(dest + i) = *(src + j);
+      ++j;
+    } else
----------------
There are two modes for computing `strncpy`:


  # copy up until `\0` is found
  # fill with `\0`

You can only ever go from 1 to 2, never the opposite.
`j` is only used in 1 and is incremented at each loop iteration.

So it seems to me that you don't need `j`, you can just use `i`.

I've been fiddling a bit with different implementations (https://godbolt.org/z/Eb9xq9) and the following one seems simple and nice
```
char *strncpy(char *__restrict dest, const char *__restrict src, size_t n) {
  size_t i = 0;
  for (; i < n && src[i] != '\0'; ++i) dest[i] = src[i];
  for (; i < n; ++i) dest[i] = '\0';
  return dest;
}
```

Added bonus, the compiler recognizes `for (; i < n; ++i) dest[i] = '\0';` as a `memset` which lowers this function's footprint. IMHO the filling with zero part is not the most used part of the algorithm so it's nice to outline it.


================
Comment at: libc/src/string/strncpy.cpp:24
+    } else
+      *(dest + i) = '\0';
+  }
----------------
Can you use `dest[i]` instead of `*(dest + i)`? I believe it's more readable.
Here and above (same for `src`).


================
Comment at: libc/test/src/string/strncpy_test.cpp:44
+  ASSERT_STREQ(lt_dest, lt);
+}
----------------
Would you mind splitting this into 3 tests to ease readability?
Also a test fixture like so would improve readability.
```
struct StrncpyFixture : public __llvm_libc::testing::Test {
  void check_strncpy(__llvm_libc::cpp::MutableArrayRef<char> dst,
                     __llvm_libc::cpp::ArrayRef<char> src, size_t n,
                     __llvm_libc::cpp::ArrayRef<char> expected) {
    // Making sure we don't overflow buffer.
    ASSERT_GE(dst.size(), n);
    // Making sure strncpy returns dst.
    ASSERT_EQ(__llvm_libc::strncpy(dst.data(), src.data(), n), dst.data());
    // Expected must be of the same size as Dest.
    ASSERT_EQ(dst.size(), expected.size());
    for (size_t i = 0; i < expected.size(); ++i)
      ASSERT_EQ(expected[i], dst[i]);
  }
};

TEST_F(StrncpyFixture, Untouched) {
  char dst[] = {'a', 'b'};
  char src[] = {'x', '\0'};
  char expected[] = {'a', 'b'};
  check_strncpy(dst, src, 0, expected);
}

TEST_F(StrncpyFixture, CopyOne) {
  char dst[] = {'a', 'b'};
  char src[] = {'x', 'y'};
  char expected[] = {'x', 'b'}; // no \0 is appended
  check_strncpy(dst, src, 1, expected);
}

TEST_F(StrncpyFixture, CopyNull) {
  char dst[] = {'a', 'b'};
  char src[] = {'\0', 'y'};
  char expected[] = {'\0', 'b'};
  check_strncpy(dst, src, 1, expected);
}

TEST_F(StrncpyFixture, CopyPastSrc) {
  char dst[] = {'a', 'b'};
  char src[] = {'\0', 'y'};
  char expected[] = {'\0', '\0'};
  check_strncpy(dst, src, 2, expected);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91399