[libc-commits] [PATCH] D135322: [libc] add strsignal and refactor message mapping

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

The new `MsgMapper` LGTM but I have comments about other parts of the design.



================
Comment at: libc/src/__support/StringUtil/error_to_string.cpp:194
+
+cpp::string_view get_error_string(int err_num, cpp::string_view buffer) {
+  auto opt_str = internal::error_mapper.get_str(err_num);
----------------
It seems to me like we should not have this overload in the `stringutil` at all because the actual libc functions need to set errno. Instead, provide a function like:

```
bool fill_unknown_error_msg(int err_num, cpp::span<char> buffer) {
   ...
  // return true if buffer was big enough, else false.
  // This will help strerror_r set errno to ERANGE based on the return value.
}
```

So, `strerror` should be implemented like this:

```
LLVM_LIBC_FUNCTION(... strerror ...) {
  auto err_str = get_error_string(num);
  if (num)
    return const_cast<char *>(num.data());
  errno = EINVAL;
  // unknown_err_buffer is declared in strerror.cpp.
  // In case of strerror_r, it is instantiated from the buffer argument.
  fill_unknown_err_msg(num, unknown_err_buffer);
  return unknown_err_buffer.data();
}
```


================
Comment at: libc/src/__support/StringUtil/message_mapper.h:23
+
+public:
+  constexpr MsgMapping(int init_num, const char *init_msg)
----------------
This is a struct so `public` is default.


================
Comment at: libc/src/__support/StringUtil/message_mapper.h:68
+        msg_offsets[num] != -1) {
+      return {const_cast<char *>(string_array + msg_offsets[num])};
+    } else {
----------------
Why is the `const_cast` required?


================
Comment at: libc/src/__support/StringUtil/signal_to_string.cpp:119
+//   }
+// };
+
----------------
Delete the commented out blocks above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135322