This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add swab implementation
ClosedPublic

Authored by Caslyn on Apr 10 2023, 1:36 PM.

Details

Summary

Swab implementation is added to libc/src/unistd.

Diff Detail

Event Timeline

Caslyn created this revision.Apr 10 2023, 1:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 10 2023, 1:36 PM
Caslyn requested review of this revision.Apr 10 2023, 1:36 PM
Caslyn updated this revision to Diff 512244.Apr 10 2023, 1:42 PM

Fix typo

michaelrj added inline comments.
libc/spec/posix.td
550

this should be SSizeTType

libc/src/unistd/linux/swab.cpp
18

this should be ssize_t

Caslyn updated this revision to Diff 512245.Apr 10 2023, 1:56 PM

Fix: size_t -> ssize_t

Caslyn marked 2 inline comments as done.Apr 10 2023, 1:59 PM
Caslyn added inline comments.
libc/src/unistd/linux/swab.cpp
18

Thanks for catching - included size check as well.

mcgrathr added inline comments.
libc/test/src/unistd/swab_test.cpp
15

Since the argument type is ssize_t there should be a test that a negative size is also a no-op.

20

This only tests the compiler, not the function, since it won't do anything to change what pointer to is.
A more meaningful test might be to have nonempty buffers with known contents and verify that the contents weren't changed (and that the from and to bytes don't match after swapping so you could tell).
It's separately worth testing that zero or negative size works with nullptr buffers since otherwise that would crash when it shouldn't, which might not be tested by the case of real buffers that weren't touched.

27

Same logic applies here. This isn't really testing anything except that it didn't crash.

33

Just = {}; is equivalent here and seems more clear that it's simply zero-initializing the whole array.

Caslyn updated this revision to Diff 512271.Apr 10 2023, 3:03 PM
Caslyn marked 5 inline comments as done.

Improve tests per review feedback

mcgrathr accepted this revision.Apr 10 2023, 3:56 PM

lgtm!

libc/src/unistd/linux/swab.cpp
18

I don't see any reason for this short-circuit. The loop will naturally have zero iterations for these cases.

This revision is now accepted and ready to land.Apr 10 2023, 3:56 PM
Caslyn updated this revision to Diff 512278.Apr 10 2023, 3:58 PM
Caslyn marked an inline comment as done.

Remove redundant check

This revision was automatically updated to reflect the committed changes.

Sorry I failed to notice the placement and build plumbing are not right here. It should not be in the linux/ subdir, it has nothing to do with linux or any other OS.
Follow the model of e.g. getopt that is in unistd but is not OS-specific.