This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add strlen implementation.
ClosedPublic

Authored by PaulkaToast on Apr 1 2020, 5:09 PM.

Details

Summary

Along with the parent patch, this should fix the last call to a non internal libc function.

Diff Detail

Event Timeline

PaulkaToast created this revision.Apr 1 2020, 5:09 PM
xbolva00 added inline comments.
libc/src/string/strcat.cpp
17โ€“18

Remove outdated comment

PaulkaToast marked 2 inline comments as done.
PaulkaToast added inline comments.
libc/src/string/strcat.cpp
17โ€“18

Good catch, thanks!

PaulkaToast marked an inline comment as done.
abrachet added inline comments.Apr 1 2020, 6:42 PM
libc/src/string/strcpy.cpp
17

We could make these __restrict while we're here but I think it there might be no benefit given we just call memcpy.

libc/src/string/strlen.cpp
17

Brackets not common for single statement loops.

libc/src/string/strlen.h
1

We could add the -*- C++ -*-

libc/test/src/string/strlen_test.cpp
17

Maybe 0UL and 12UL whichever you think looks better.

PaulkaToast marked 4 inline comments as done.
PaulkaToast added inline comments.
libc/src/string/strlen.h
1

Just noticed none of the headers in string have this. I'll go make a separate patch for that. Thanks!

libc/test/src/string/strlen_test.cpp
17

Ah, good point! I think I prefer the explicit cast because its very obvious what type it is.

sivachandra accepted this revision.Apr 3 2020, 11:08 AM
sivachandra marked an inline comment as done.

Couple of nits inline but LGTM.

libc/src/string/strlen.cpp
15

Can you add a TODO here saying that one could improve this by making use of the alignment and target machine word size information?

libc/src/string/strlen.h
1

This problem is probably more wide spread than just the string directory. +1 to fixing them all up in a separate, may be obvious patch.

13

Do you still need to include stddef.h if you include include/string.h? In my patches, I have included the public header iff required. For example, the public header defines a header which is required by the API declared here. So, keeping stddef.h and removing include/string.h is also fine. Whatever you choose, share your thoughts so that I can learn if I am missing something.

This revision is now accepted and ready to land.Apr 3 2020, 11:08 AM
sivachandra added inline comments.Apr 3 2020, 11:24 AM
libc/src/string/strlen.h
13

I mean't to say, "For example, if the public header defines a *struct* which is required by the API declared here."

PaulkaToast marked 2 inline comments as done.
PaulkaToast added inline comments.
libc/src/string/strlen.cpp
15

Added a more general comment about looking into the performance of this function.

libc/src/string/strlen.h
13

Thanks for bringing this up! I based this off the memcpy header without much consideration.

Now that I think about it more, it seems that both strlen and memcpy indirectly include stddef.h from include/string.h. I think it makes more sense to use the include/string.h approach as that will be more consistent and would require less code duplication in terms of figuring out which entrypoints depend on which headers.

PaulkaToast marked an inline comment as done.Apr 3 2020, 1:43 PM
PaulkaToast added inline comments.
libc/src/string/strlen.h
1

Agreed I can send something out for this shortly.

PaulkaToast marked 6 inline comments as done.
PaulkaToast added inline comments.
libc/src/string/strlen.cpp
18

This is great suggestion thank you! (:

This revision was automatically updated to reflect the committed changes.