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

[Michael Jones via Phabricator via libc-commits]

michaelrj marked an inline comment as done.
michaelrj added inline comments.


================
Comment at: libc/src/__support/cstring.h:21
+  cpp::string str;
+  const char *cstr;
+
----------------
sivachandra wrote:
> The comment trail is on a different diff, but let me restart it here.
> 
> When you have:
> 
> ```
>   cpp::string str("abc");
> ```
> 
> The var `str` holds a copy of `"abc"`. Which means that `str` can be destroyed safely without worrying about the `cpp::string`'s desctructor trying to free its resources. So, you shouldn't need `cstr` and also not need the constructor on line 34. If you have code like this:
> 
> ```
>   CString str("abc");
> ```
> 
> The constructor on line 29 will be called as `const char *` is implicitly convertible to `cpp::string`: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/string.h#L61.
> 
> The reason I suggested a template is because, in a general case, we should not try to copy `const char *` strings. You can do that adjustment later.
In my testing I'm not seeing the behavior you're describing. When I debug `fprintf_test.cpp` and look at which constructor `CmakeFilePath.cpp` is calling I see it using the one on line 34. I think what's happening is the compiler sees that the only call to `CString` is with a `const char*` and the version on line 29 is getting removed as unused from the final code since it's all inlined. Regardless, it's not constructing a non-trivial `cpp::string`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147008