[libc-commits] [PATCH] D147967: [libc] move strerror and strsignal to OS msg maps

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/__support/StringUtil/error_to_string.cpp:18
+#ifdef __unix__
+#include "src/__support/StringUtil/linux/strerror_map.h"
+#else
----------------
sivachandra wrote:
> sivachandra wrote:
> > Instead of expecting everything to be listed in a platform specific table, we can have three kinds of tables:
> > 
> > 1. Table of errors specified by the C standard - this will be the fallback table instead of the `#error` you have below.
> > 2. Table of POSIX extension errors.
> > 3. Table of Linux extension errors.
> > 
> > To get the full Linux table for example, you will have to merge all the above three tables. You likely have to use `cpp::array` for the tables to get this to work at compile-time.
> I prefer an organization like this:
> 
> 1. `stdc_error_table.h` contains:
> ```
> inline constexpr ErrorTable<...> STDC_ERRORS = {
>   ...
> };
> ```
> 2. `posix_error_table.h` contains:
> ```
> inline constexpr ErrorTable<...> POSIX_EXTENSION_ERRORS = {
>   ...
> };
> ```
> 3. `linux/error_table.h` contains:
> ```
> #include "stdc_errors.h"
> #include "posix_errros.h"
> 
> namespace __llvm_libc {
> inline constexpr ErrorTable<...> LINUX_EXTENSION_ERRORS = {
>   ...
> };
> } // namespace __llvm_libc
> ```
> 4. `error_table.h` contains:
> ```
> #ifdef __linux__
> #include "linux/error_table.h"
> inline constexpr ErrorTable<...> PLATFORM_ERRORS = STDC_ERRORS + POSIX_EXTENSION_ERRORS + LINUX_EXTENSION_ERRORS;
> #else
> #include "stdc_error_table.h"
> inline constexpr ErrorTable<...> PLATFORM_ERRORS = STDC_ERRORS;
> #endif
> ```
> 
> In this file, all you have to do is include `error_table.h` and operate on `PLATFORM_ERRORS`.
I moved these files into a subdirectory for clarity, but otherwise this is laid out as described.


================
Comment at: libc/src/__support/StringUtil/message_mapper.h:31
 
+template <class T, size_t N> class ArrayConcat {
+  T Data[N];
----------------
sivachandra wrote:
> Do we need a special class? Can this work:
> 
> ```
> template <size_t N>
> using ErrorTable = cpp::array<MsgMapping, N>;
> 
> template <size_t N1, size_t N2>
> constexpr ErrorTable<N1 + N2> operator+(const ErrorTable &t1, const ErrorTable &t2) {
>   ErrorTable<N1 + N2> res{};
>   for (std::size_t i = 0; i < N1; ++i)
>     res[i] = t1[i];
>   for (std::size_t i = 0; i < N2; ++i)
>     res[N1 + i] = t2[i];
>   return res;
> }
> ```
Interesting, this does work, but when I tried adding `operator+` to `cpp::array` directly it complained about the result not being constexpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147967