[libcxx-commits] [PATCH] D103339: [libcxx][NFC] Tidy up calculation of __nbuf in num_put::do_put, and add comments

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM created this revision.
DanielMcIntosh-IBM added reviewers: dim, mclow.lists, EricWF.
DanielMcIntosh-IBM requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

In 07ef8e679621 and 3ed9f6ebdeeb, __nbuf started to diverge from the amount of
space that was actually needed for the buffer. For 32-bit longs for example, we
allocate a buffer that is one larger than needed. Moreover, it is no longer
clear exactly where the extra +1 or +2 comes from - they're just numbers pulled
from thin air. This PR cleans up how __nbuf is calculated, and adds comments to
further clarify where each part comes from.

Specifically, it corrects the underestimation of the max size buffer needed
that the above two commits had to compensate for. The root cause looks to be
the use of signed type parameters to numeric_limits<>::digits. Since digits
only counts non-sign bits, the calculation was acting as though (for a signed
64-bit type) the longest value we would print was 2^63 in octal. However,
printing in octal treats values as unsigned, so it is actually 2^64. Thus,
using unsigned types and changing the final +2 to a +1 is probably a better
option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103339

Files:
  libcxx/include/locale


Index: libcxx/include/locale
===================================================================
--- libcxx/include/locale
+++ libcxx/include/locale
@@ -1458,10 +1458,12 @@
     char __fmt[6] = {'%', 0};
     const char* __len = "l";
     this->__format_int(__fmt+1, __len, true, __iob.flags());
-    const unsigned __nbuf = (numeric_limits<long>::digits / 3)
-                          + ((numeric_limits<long>::digits % 3) != 0)
-                          + ((__iob.flags() & ios_base::showbase) != 0)
-                          + 2;
+    // Worst case is octal, with showbase enabled. Note that octal is always
+    // printed as an unsigned value.
+    const unsigned __nbuf = (numeric_limits<unsigned long>::digits / 3)          // 1 char per 3 bits
+                            + ((numeric_limits<unsigned long>::digits % 3) != 0) // round up
+                            + ((__iob.flags() & ios_base::showbase) != 0)        // base prefix
+                            + 1;                                                 // terminating null character
     char __nar[__nbuf];
     int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
     char* __ne = __nar + __nc;
@@ -1485,10 +1487,12 @@
     char __fmt[8] = {'%', 0};
     const char* __len = "ll";
     this->__format_int(__fmt+1, __len, true, __iob.flags());
-    const unsigned __nbuf = (numeric_limits<long long>::digits / 3)
-                          + ((numeric_limits<long long>::digits % 3) != 0)
-                          + ((__iob.flags() & ios_base::showbase) != 0)
-                          + 2;
+    // Worst case is octal, with showbase enabled. Note that octal is always
+    // printed as an unsigned value.
+    const unsigned __nbuf = (numeric_limits<unsigned long long>::digits / 3)          // 1 char per 3 bits
+                            + ((numeric_limits<unsigned long long>::digits % 3) != 0) // round up
+                            + ((__iob.flags() & ios_base::showbase) != 0)             // base prefix
+                            + 1;                                                      // terminating null character
     char __nar[__nbuf];
     int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
     char* __ne = __nar + __nc;
@@ -1512,10 +1516,11 @@
     char __fmt[6] = {'%', 0};
     const char* __len = "l";
     this->__format_int(__fmt+1, __len, false, __iob.flags());
-    const unsigned __nbuf = (numeric_limits<unsigned long>::digits / 3)
-                          + ((numeric_limits<unsigned long>::digits % 3) != 0)
-                          + ((__iob.flags() & ios_base::showbase) != 0)
-                          + 1;
+    // Worst case is octal, with showbase enabled.
+    const unsigned __nbuf = (numeric_limits<unsigned long>::digits / 3)          // 1 char per 3 bits
+                            + ((numeric_limits<unsigned long>::digits % 3) != 0) // round up
+                            + ((__iob.flags() & ios_base::showbase) != 0)        // base prefix
+                            + 1;                                                 // terminating null character
     char __nar[__nbuf];
     int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
     char* __ne = __nar + __nc;
@@ -1539,10 +1544,11 @@
     char __fmt[8] = {'%', 0};
     const char* __len = "ll";
     this->__format_int(__fmt+1, __len, false, __iob.flags());
-    const unsigned __nbuf = (numeric_limits<unsigned long long>::digits / 3)
-                          + ((numeric_limits<unsigned long long>::digits % 3) != 0)
-                          + ((__iob.flags() & ios_base::showbase) != 0)
-                          + 1;
+    // Worst case is octal, with showbase enabled.
+    const unsigned __nbuf = (numeric_limits<unsigned long long>::digits / 3)          // 1 char per 3 bits
+                            + ((numeric_limits<unsigned long long>::digits % 3) != 0) // round up
+                            + ((__iob.flags() & ios_base::showbase) != 0)             // base prefix
+                            + 1;                                                      // terminating null character
     char __nar[__nbuf];
     int __nc = __libcpp_snprintf_l(__nar, sizeof(__nar), _LIBCPP_GET_C_LOCALE, __fmt, __v);
     char* __ne = __nar + __nc;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103339.348579.patch
Type: text/x-patch
Size: 4357 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20210528/4de5f390/attachment.bin>