[libc-commits] [PATCH] D147970: [libc] Add swab implementation

[Roland McGrath via Phabricator via libc-commits]

mcgrathr added inline comments.


================
Comment at: libc/test/src/unistd/swab_test.cpp:14
+#include "test/UnitTest/Test.h"
+
+TEST(LlvmLibcSwabTest, ZeroSizeIsNoOpt) {
----------------
Since the argument type is `ssize_t` there should be a test that a negative size is also a no-op.



================
Comment at: libc/test/src/unistd/swab_test.cpp:19
+  __llvm_libc::swab(from, to, 0);
+  ASSERT_EQ(to, static_cast<void *>(nullptr));
+}
----------------
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.



================
Comment at: libc/test/src/unistd/swab_test.cpp:26
+  __llvm_libc::swab(from, to, sizeof(from));
+  ASSERT_EQ(to, static_cast<void *>(nullptr));
+}
----------------
Same logic applies here. This isn't really testing anything except that it didn't crash.



================
Comment at: libc/test/src/unistd/swab_test.cpp:32
+    const char *from = "ab";
+    char to[3] = {'\0'};
+    __llvm_libc::swab(from, to, __llvm_libc::internal::string_length(from));
----------------
Just `= {};` is equivalent here and seems more clear that it's simply zero-initializing the whole array.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147970