[libc-commits] [PATCH] D147008: [libc][bazel] add file printf targets and support

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/test/UnitTest/BazelFilePath.cpp:19
+// This is the path to the folder bazel wants the test outputs written to.
+static const char *const UNDECLARED_OUTPUTS_PATH =
+    getenv("TEST_UNDECLARED_OUTPUTS_DIR");
----------------
We should ideally avoid depending on global object initialization. Can we call `getenv` from inside the function below?


================
Comment at: libc/test/UnitTest/CmakeFilePath.cpp:20
+libc_make_test_file_path_func(const char *file_name) {
+  return cpp::string(PREFIX) + file_name;
+}
----------------
Do we really need the `testdata/` prefix? As in, can we not just create a file in the same directory as the test? It makes this function a simple pass through.


================
Comment at: libc/utils/testutils/OwnedString.h:14
+
+template <typename Str_T> class OwnedString {
+  Str_T str;
----------------
Why is this class required?


================
Comment at: utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl:13
 
+PRINTF_COPTS = [
+    "-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE",
----------------
These should be defined in `libc/BUILD.bazel`. Also, they should passed to libc_function as `defines = PRINTF_COPTS`, which then will be passed to tests which depend on those functions which use those defines.


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl:17
 
-def libc_test(name, srcs, libc_function_deps, deps = [], **kwargs):
+def libc_test(name, srcs, libc_function_deps, deps = [], output_file = "", **kwargs):
     """Add target for a libc test.
----------------
Unused?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147008