[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/cstring.h:21
+  cpp::string str;
+  const char *cstr;
+
----------------
michaelrj wrote:
> 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`.
> 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. 

That is because you have that constructor. Are you facing any problem if you remove that constructor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147008