[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/src/__support/CPP/OwnedString.h:1
+//===-- Implementation of a struct to hold a string in menory -------------===//
+//
----------------
This class should live outside of the `CPP` directory.


================
Comment at: libc/src/__support/CPP/OwnedString.h:19
+// for if that char* needs to be freed.
+class OwnedString {
+  string str;
----------------
You can make this a template class like this:

```
template <typename T>
class CString {
  T str;
public:
  // These constructors can be implemented iff required.
  CString() = delete;
  CString(const CString &) = delete;
  CString(CString &&) = delete;

  LIBC_INLINE CString(const T &s) : str(s) {}
  LIBC_INLINE operator const char *() const { return str.data(); }
};

template <>
class CString<const char *> {
  const char *str;
public:
  // These constructors can be implemented iff required.
  CString() = delete;
  CString(const CString &) = delete;
  CString(CString &&) = delete;

  LIBC_INLINE CString(const char *s) : s(str) {}
  LIBC_INLINE operator const char *() const { return str; }
};
```

The template will work with `cpp::string_view` and `cpp::span` also.


================
Comment at: libc/test/src/stdio/fprintf_test.cpp:38
+  const char *FILENAME = "fprintf_output.test";
+  auto FILE_PATH = libc_make_test_file_path(FILENAME);
+
----------------
This comment is for the `OwnedString` de-allocation problem you have explained.

This is working for you because `FILE_PATH` is actually a variable and not a temporary. Had you used like, 

```
const char *FILE_PATH = libc_make_test_file_path(FILENAME);
::FILE *file = printf_test::fopen(FILE_PATH, "w");
```

on line 40, you would had the same de-allocation problem. Put other way around, if you make `FILE_PATH` a `cpp::string` instance, de-allocation will not happen until the end of this test's scope. The ability to implicitly convert it to a `const char *` value is still desirable, so I have added a comment under `OwnedString`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147008