Page MenuHomePhabricator

[libcxx][NFC] Tidy up calculation of __nbuf in num_put::do_put, and add comments
ClosedPublic

Authored by DanielMcIntosh-IBM on May 28 2021, 1:37 PM.

Details

Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
1,910 mslibcxx CI GCC/C++20 > libc++.std/concepts/concepts_callable/concept_invocable::invocable.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++ /home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/libcxx/test/std/concepts/concepts.callable/concept.invocable/invocable.compile.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++20 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only
1,930 mslibcxx CI GCC/C++20 > libc++.std/concepts/concepts_callable/concept_regularinvocable::regular_invocable.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++ /home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/libcxx/test/std/concepts/concepts.callable/concept.regularinvocable/regular_invocable.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++20 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -L/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/e3112dc96de4-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/test/std/concepts/concepts.callable/concept.

Event Timeline

DanielMcIntosh-IBM requested review of this revision.May 28 2021, 1:37 PM
DanielMcIntosh-IBM created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2021, 1:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
DanielMcIntosh-IBM edited the summary of this revision. (Show Details)May 28 2021, 1:39 PM

Nice catch! Can you rebase the patch to rerun the CI?

libcxx/include/locale
1466

Wouldn't it look better to do the rounded up character determination in one step?
((numeric_limits<unsigned long>::digits + 2) / 3)

ldionne accepted this revision.May 31 2021, 1:45 PM
ldionne added a subscriber: ldionne.

Thanks for fixing this! The CI is failing due to something that has been fixed now. Please rebase onto main and re-upload your diff, that will run CI again and everything should be green.

libcxx/include/locale
1466

IMO it's easier to read as-is since the + 2 trick isn't immediately obvious (to me at least).

This revision is now accepted and ready to land.May 31 2021, 1:45 PM
Mordante added inline comments.May 31 2021, 11:12 PM
libcxx/include/locale
1466

I thought this was very common idiom for rounding up. But if it's not obvious for everybody, then I'm in favour of keeping the code as is.

Mordante accepted this revision.Jun 1 2021, 11:04 PM

rebase

Thanks, LGTM!