[libc-commits] [PATCH] D134074: [libc] add strerror

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/error_to_string.h:1
+//===-- Definition of a class for mapping errors to strings -----*- C++ -*-===//
+//
----------------
michaelrj wrote:
> sivachandra wrote:
> > Almost everything in this file should live in a `.cpp` file. The only thing declared in the .h file should something like:
> > 
> > ```
> > cpp::string_view error_to_string(int error);
> > ```
> > 
> > IMO, that single function should take care of unknown error numbers also. Do you have a use case where `max_buff_size` and `get_unknown_str` are required?
> > 
> > A more fundamental question - is there any reason why this should live in `__support` at all?
> I did make a .cpp file to contain the functions for the class, but was running into compile errors where the linker couldn't find the implementations, so I moved it into the header for now.
> 
> As for the other parts:
> The reason `max_buff_size` and `get_unknown_str` exist is that when an error without a specified string mapping is passed, we need to generate a new string that says `Unknown error [number]`. Since this string will be passed back to the caller code, it can't be on the stack, so we need a buffer to hold it. In `strerror.cpp` I define a global buffer of size `max_buff_size` to be that buffer. This is specifically allowed by the standard: "The strerror function returns a pointer to the string, the contents of which are localespecific. The array pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the strerror function."
> 
> I placed this in support because both `strerror` which is in strings.h and `perror` which is in stdio.h use the same functionality. I can move it into strings, but generally I try to avoid including files between headers when possible.
> I did make a .cpp file to contain the functions for the class, but was running into compile errors where the linker couldn't find the implementations, so I moved it into the header for now.

That shouldn't happen - we have lots of examples where in we store tables in `.cpp` files, especially in the math directory.

> As for the other parts:
> The reason `max_buff_size` and `get_unknown_str` exist is that when an error without a specified string mapping is passed, we need to generate a new string that says `Unknown error [number]`. Since this string will be passed back to the caller code, it can't be on the stack, so we need a buffer to hold it. In `strerror.cpp` I define a global buffer of size `max_buff_size` to be that buffer. This is specifically allowed by the standard: "The strerror function returns a pointer to the string, the contents of which are localespecific. The array pointed to shall not be modified by the program, but may be overwritten by a subsequent call to the strerror function."

My question was more about, why should, say the `strerror` implementation, provide its own error buffer. Instead, why can't all these details be hidden behind a single `error_to_string`. Also, since `errno` is a thread local value, the error string buffer should also be thread local.

> I placed this in support because both `strerror` which is in strings.h and `perror` which is in stdio.h use the same functionality. I can move it into strings, but generally I try to avoid including files between headers when possible.

SGTM




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134074