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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/StringUtil/CMakeLists.txt:3
+  return()
+endif()
+
----------------
I don't think we should just return when a platform directory is missing. See below.


================
Comment at: libc/src/__support/StringUtil/CMakeLists.txt:15
+# The individual maps depend on message_mapper.
+add_subdirectory(${LIBC_TARGET_OS})
+
----------------
This should be conditional:

```
if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
  add_subdirectory(${LIBC_TARGET_OS})
endif()
```


================
Comment at: libc/src/__support/StringUtil/CMakeLists.txt:30
   libc.src.__support.integer_to_string
+  libc.src.__support.StringUtil.${LIBC_TARGET_OS}.strerror_map
 )
----------------
This should be added conditionally I think:

```
list(APPEND error_to_string_deps 
  libc.src.errno.errno
  libc.src.__support.CPP.span
  libc.src.__support.CPP.string_view
  libc.src.__support.CPP.stringstream
  libc.src.__support.integer_to_string
)
if(TARGET libc.src.__support.StringUtil.${LIBC_TARGET_OS}.strerror_map)
  list(APPEND error_to_string_deps  libc.src.__support.StringUtil.${LIBC_TARGET_OS}.strerror_map)
else()
  list(APPEND error_to_string_deps  <target for the default error map - see below>)
endif()
```


================
Comment at: libc/src/__support/StringUtil/CMakeLists.txt:47
   libc.src.__support.integer_to_string
+  libc.src.__support.StringUtil.${LIBC_TARGET_OS}.strsignal_map
 )
----------------
Ditto.


================
Comment at: libc/src/__support/StringUtil/error_to_string.cpp:17
+
+#ifdef __unix__
+#include "src/__support/StringUtil/linux/strerror_map.h"
----------------
s/`__unix__`/`__linux__` ?


================
Comment at: libc/src/__support/StringUtil/error_to_string.cpp:18
+#ifdef __unix__
+#include "src/__support/StringUtil/linux/strerror_map.h"
+#else
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147967