[libc-commits] [libc] [libc] fix -Wtype-limits in wctob (PR #74511)

[Nick Desaulniers via libc-commits]

https://github.com/nickdesaulniers updated https://github.com/llvm/llvm-project/pull/74511

>From 4d7a8b10788ecd6bd747f7d4c99a491e3fd19b59 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Tue, 5 Dec 2023 10:31:00 -0800
Subject: [PATCH 1/5] [libc] disable -Wtype-limits in wctob
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GCC warns:
libc/src/__support/wctype_utils.h:33:20: error: comparison of unsigned
expression in ‘< 0’ is always false [-Werror=type-limits]
   33 |   if (c > 127 || c < 0)
      |                  ~~^~~

Looking into the signedness of wint_t, it looks like depending on the platform,
__WINT_TYPE__ is defined to int or unsigned int depending on the platform.

Link: https://lab.llvm.org/buildbot/#/builders/250/builds/14891/steps/6/logs/stdio
---
 libc/src/__support/wctype_utils.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libc/src/__support/wctype_utils.h b/libc/src/__support/wctype_utils.h
index 6d825499a1b0f..367f5a86647fd 100644
--- a/libc/src/__support/wctype_utils.h
+++ b/libc/src/__support/wctype_utils.h
@@ -29,9 +29,14 @@ LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
   // This needs to be translated to EOF at the callsite. This is to avoid
   // including stdio.h in this file.
   // The standard states that wint_t may either be an alias of wchar_t or
-  // an alias of an integer type, so we need to keep the c < 0 check.
+  // an alias of an integer type where different platforms define this type with
+  // different signedness, so we need to keep the c < 0 check, hence the
+  // pragmas.
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wtype-limits"
   if (c > 127 || c < 0)
     return cpp::nullopt;
+#pragma GCC diagnostic pop
   return static_cast<int>(c);
 }
 

>From f3a22d435ec559ca43b3bd55fb3be10c6eb6e071 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Tue, 5 Dec 2023 11:29:26 -0800
Subject: [PATCH 2/5] use cpp::is_signed_v && update comment

---
 libc/src/__support/wctype_utils.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/libc/src/__support/wctype_utils.h b/libc/src/__support/wctype_utils.h
index 367f5a86647fd..4253f3682c5d1 100644
--- a/libc/src/__support/wctype_utils.h
+++ b/libc/src/__support/wctype_utils.h
@@ -28,15 +28,14 @@ namespace internal {
 LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
   // This needs to be translated to EOF at the callsite. This is to avoid
   // including stdio.h in this file.
-  // The standard states that wint_t may either be an alias of wchar_t or
-  // an alias of an integer type where different platforms define this type with
-  // different signedness, so we need to keep the c < 0 check, hence the
-  // pragmas.
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wtype-limits"
-  if (c > 127 || c < 0)
+  if (c > 127)
     return cpp::nullopt;
-#pragma GCC diagnostic pop
+  // The standard states that wint_t may either be an alias of wchar_t or
+  // an alias of an integer type, different platforms define this type with
+  // different signedness, so we may need the c < 0 check.
+  if constexpr (cpp::is_signed_v<wint_t>)
+    if (c < 0)
+      return cpp::nullopt;
   return static_cast<int>(c);
 }
 

>From 0617a61e74faaedc04a3bc7c411e9fa6e8f8cfcd Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Tue, 5 Dec 2023 13:04:22 -0800
Subject: [PATCH 3/5] templatize wctob

---
 libc/src/__support/wctype_utils.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libc/src/__support/wctype_utils.h b/libc/src/__support/wctype_utils.h
index 4253f3682c5d1..0641b70d80886 100644
--- a/libc/src/__support/wctype_utils.h
+++ b/libc/src/__support/wctype_utils.h
@@ -25,6 +25,7 @@ namespace internal {
 // of a function call by inlining them.
 // ------------------------------------------------------
 
+template<typename W = wint_t>
 LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
   // This needs to be translated to EOF at the callsite. This is to avoid
   // including stdio.h in this file.
@@ -33,7 +34,7 @@ LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
   // The standard states that wint_t may either be an alias of wchar_t or
   // an alias of an integer type, different platforms define this type with
   // different signedness, so we may need the c < 0 check.
-  if constexpr (cpp::is_signed_v<wint_t>)
+  if constexpr (cpp::is_signed_v<W>)
     if (c < 0)
       return cpp::nullopt;
   return static_cast<int>(c);

>From 1d761ff894542303f37c063a36424ecd6a9d9716 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Tue, 5 Dec 2023 13:15:39 -0800
Subject: [PATCH 4/5] git clang-format HEAD~

---
 libc/src/__support/wctype_utils.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libc/src/__support/wctype_utils.h b/libc/src/__support/wctype_utils.h
index 0641b70d80886..4b211aab02f4d 100644
--- a/libc/src/__support/wctype_utils.h
+++ b/libc/src/__support/wctype_utils.h
@@ -25,8 +25,7 @@ namespace internal {
 // of a function call by inlining them.
 // ------------------------------------------------------
 
-template<typename W = wint_t>
-LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
+template <typename W = wint_t> LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
   // This needs to be translated to EOF at the callsite. This is to avoid
   // including stdio.h in this file.
   if (c > 127)

>From 44406f87f465fb2ea5d292801b50a18c9fabca75 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers at google.com>
Date: Tue, 5 Dec 2023 15:12:35 -0800
Subject: [PATCH 5/5] remove templates, use Schrodinger's suggestion

---
 libc/src/__support/wctype_utils.h | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/libc/src/__support/wctype_utils.h b/libc/src/__support/wctype_utils.h
index 4b211aab02f4d..aa1161c777453 100644
--- a/libc/src/__support/wctype_utils.h
+++ b/libc/src/__support/wctype_utils.h
@@ -25,17 +25,15 @@ namespace internal {
 // of a function call by inlining them.
 // ------------------------------------------------------
 
-template <typename W = wint_t> LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
+LIBC_INLINE cpp::optional<int> wctob(wint_t c) {
   // This needs to be translated to EOF at the callsite. This is to avoid
   // including stdio.h in this file.
-  if (c > 127)
-    return cpp::nullopt;
   // The standard states that wint_t may either be an alias of wchar_t or
   // an alias of an integer type, different platforms define this type with
-  // different signedness, so we may need the c < 0 check.
-  if constexpr (cpp::is_signed_v<W>)
-    if (c < 0)
-      return cpp::nullopt;
+  // different signedness. This is equivalent to `(c > 127) || (c < 0)` but also
+  // works without -Wtype-limits warnings when `wint_t` is unsigned.
+  if ((c & ~127) != 0)
+    return cpp::nullopt;
   return static_cast<int>(c);
 }