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

[Michael Jones via Phabricator via libc-commits]

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


================
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);
----------------
michaelrj wrote:
> sivachandra wrote:
> > 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();
> > }
> > ```
> For comments on the `strerror_r` design you should comment on this patch instead: https://reviews.llvm.org/D135227 but overall I think the idea makes sense.
after further testing, it appears that strerror and strerror_r don't actually set errno in existing implementations. Given that information, I don't think this is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135322