This is an archive of the discontinued LLVM Phabricator instance.

[libc] Adds implementation for memrchr.
ClosedPublic

Authored by cgyurgyik on Jul 23 2020, 3:39 PM.

Diff Detail

Event Timeline

cgyurgyik created this revision.Jul 23 2020, 3:39 PM
cgyurgyik updated this revision to Diff 280275.Jul 23 2020, 3:41 PM
lntue added a subscriber: lntue.Jul 23 2020, 4:05 PM
lntue added inline comments.
libc/src/string/memrchr.cpp
19

I think this will be a trouble if n == 0 because n is unsigned.

cgyurgyik added inline comments.Jul 23 2020, 4:58 PM
libc/src/string/memrchr.cpp
19

You're right. Good catch, thanks.

cgyurgyik updated this revision to Diff 280299.Jul 23 2020, 5:17 PM
cgyurgyik updated this revision to Diff 280300.
cgyurgyik marked 3 inline comments as done.Jul 23 2020, 5:19 PM
cgyurgyik added inline comments.
libc/src/string/memrchr.cpp
19

Fixed, and added a test where size is 0.

This is a GNU extension. So, you need to add entry to https://github.com/llvm/llvm-project/blob/master/libc/spec/gnu_ext.td and linux's api.td.

libc/src/string/memrchr.cpp
19

For better readability, may be:

for (; n != 0; --n) {
  ...
}
cgyurgyik marked an inline comment as done.Jul 24 2020, 10:36 AM
cgyurgyik added inline comments.
libc/src/string/memrchr.cpp
19

Are you looking for something along these lines:

for (; n != 0; --n) {
      if (str[n] == ch) return const_cast<unsigned char *>(str + n);
  }
  return str[0] == ch ? const_cast<unsigned char *>(str) : nullptr;

Or, am I missing something simpler here?

This is a GNU extension. So, you need to add entry to https://github.com/llvm/llvm-project/blob/master/libc/spec/gnu_ext.td and linux's api.td.

Done. libc.include.string, check-libc, and libc-integration-test all passed.

sivachandra added inline comments.Jul 24 2020, 11:01 AM
libc/src/string/memrchr.cpp
19

Very close:

for (; n != 0; --n) {
  const unsigned char *s = str + n - 1;
  if (*s == ch)
    return const_cast<unsigned char *>(s);
}
return nullptr;

The post decrement you have currently makes it easy miss that n - 1 offset.

sivachandra added inline comments.Jul 24 2020, 11:08 AM
libc/config/linux/aarch64/entrypoints.txt
16

I missed in the previous reviews, but can you please sort these lists alphabetically.

cgyurgyik marked 3 inline comments as done.
cgyurgyik added inline comments.Jul 24 2020, 11:19 AM
libc/config/linux/aarch64/entrypoints.txt
16

Are you OK with me doing this in a separate commit?

sivachandra marked an inline comment as done.Jul 24 2020, 11:23 AM
sivachandra added inline comments.
libc/config/linux/aarch64/entrypoints.txt
16

OK.

sivachandra accepted this revision.Jul 24 2020, 11:23 AM
This revision is now accepted and ready to land.Jul 24 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.