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

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/CPP/string.h:116
 
+  LIBC_INLINE explicit operator char *() const { return buffer_; }
+
----------------
sivachandra wrote:
> Is this part of `std::string`, with the `explicit` tag?
no, I added it when I thought `OwnedString` would need to be templated so that both `cpp::string` and `const char*` arguments could be safely handled in the same way. I've removed it.


================
Comment at: libc/utils/testutils/OwnedString.h:14
+
+template <typename Str_T> class OwnedString {
+  Str_T str;
----------------
sivachandra wrote:
> michaelrj wrote:
> > sivachandra wrote:
> > > Why is this class required?
> > This class holds the `cpp::string` so that it doesn't get deallocated, and is implicitly castable to `char *`. 
> > 
> > In the test, the pattern is `auto FILE_PATH = libc_make_test_file_path("filepath");`, and then the `FILE_PATH` can be used as a `char *`. The actual type of `FILE_PATH` might be `char*` or `OwnedString`, but both work as `char*`. 
> > We can't just have `FILE_PATH` be a `cpp::string` because `cpp::string` isn't implicitly castable to `char*`, and I'm trying to avoid needing a static cast every time the `FILE_PATH` is used.
> > Similarly, the `FILE_PATH` can't just be a `char*` since if we don't store the string it gets deallocated.
> > The original intent of this class was for `FILE_PATH` to always be an `OwnedString`, which is why it's a template, but that's not necessary now.
> > 
> > I've also moved this to `__support/CPP/` since `LibcTest.h` says to only include headers from that folder and `testutils`.
> So, two things you say:
> 
> 1. Doesn't get deallocated.
> 2. Implicitly castable to `char *`.
> 
> I get #2 but can you explain #1 more?
By "Doesn't get deallocated" I mean that if we made `FILE_PATH` a `const char*` and static cast the return value from `libc_make_test_file_path` to `const char*` then the `cpp::string` that `libc_make_test_file_path` returns would not be stored. The value of `FILE_PATH` would be set to the pointer to the buffer inside the `cpp::string`, then the `cpp::string` would be destructed, `free`ing that memory.

`OwnedString` solves this problem by owning the `cpp::string` and its memory, keeping it from being destructed until the end of the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147008