[libc-commits] [PATCH] D74665: [libc] [UnitTest] Create death tests

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/UnitTest/CMakeLists.txt:7
 )
-target_include_directories(LibcUnitTest PUBLIC ${LIBC_SOURCE_DIR})
+target_include_directories(LibcUnitTest PUBLIC ${LIBC_SOURCE_DIR} ${LIBC_SOURCE_DIR}/utils/)
 add_dependencies(LibcUnitTest standalone_cpp)
----------------
This shouldn't be required as we want to include like `#include "utils/testutils/..."`.


================
Comment at: libc/utils/UnitTest/Test.cpp:11
 
+#include "testutils/ExecuteFunction.h"
 #include "llvm/ADT/StringRef.h"
----------------
Use `utils/...' as suggested above.


================
Comment at: libc/utils/UnitTest/Test.cpp:254
+               << "To be killed by signal: " << Signal << '\n'
+               << "              Which is: " << ::strsignal(Signal) << '\n'
+               << "  But it was killed by: " << KilledBy << '\n'
----------------
Can we avoid calling `strsignal` in this file? May be add a `signalAsString` helper function in `ExecuteFunction.h`


================
Comment at: libc/utils/UnitTest/Test.h:12
 
+#include "testutils/ExecuteFunction.h"
 #include "utils/CPP/TypeTraits.h"
----------------
Use `utils/...` as suggested above.


================
Comment at: libc/utils/testutils/ExecuteFunction.h:21
+
+struct InvokeResult {
+  int PlatformDefined;
----------------
Call this struct `ProcessStatus` as it is actually that? Otherwise, it is easy to confuse with function return value.


================
Comment at: libc/utils/testutils/ExecuteFunction.h:29
+
+InvokeResult invokeFunction(FunctionCaller *Func);
+
----------------
Name the function `invokeInSubprocess`?


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

https://reviews.llvm.org/D74665