[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;
+
----------------
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.


================
Comment at: libc/src/__support/cstring.h:29
+
+  inline CString(cpp::string in_str) {
+    str = in_str;
----------------
You should use LIBC_INLINE for code in `src/`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147008