libc/src/stdio/printf_core/float_dec_converter.h:505: Possible poor error checking ?

[dcb314]

Static analyser cppcheck says:

libc/src/stdio/printf_core/float_dec_converter.h:505:23: style: Unsigned expression 'positive_blocks' can't be negative so it is unnecessary to test it. [unsignedPositive]

Source code is

const size_t positive_blocks = float_converter.get_positive_blocks();

if (positive_blocks >= 0) {

In a strict sense, the static analyser is correct and the test looks pointless and
so could be deleted.

A deeper analysis suggests that get_positive_blocks might return 0 on error
and so the if test is doing some poor error checking and so the it should be

if (positive_block > 0) {

Someone with more knowledge of the code would be able to offer a better opinion.

git blame says Jonas Hahnfeld last changed the code in commit 99b5474,
but git blame frequently lies with shallow copies ;-<

[llvmbot]

@llvm/issue-subscribers-libc

Author: None (dcb314)

Static analyser cppcheck says:

libc/src/stdio/printf_core/float_dec_converter.h:505:23: style: Unsigned expression 'positive_blocks' can't be negative so it is unnecessary to test it. [unsignedPositive]

Source code is

const size_t positive_blocks = float_converter.get_positive_blocks();

if (positive_blocks >= 0) {

In a strict sense, the static analyser is correct and the test looks pointless and
so could be deleted.

A deeper analysis suggests that get_positive_blocks might return 0 on error
and so the if test is doing some poor error checking and so the it should be

if (positive_block > 0) {

Someone with more knowledge of the code would be able to offer a better opinion.

git blame says Jonas Hahnfeld last changed the code in commit 99b5474,
but git blame frequently lies with shallow copies ;-<

[michaelrj-google]

Thanks for finding this. After inspecting the code a bit it seems that the if was effectively unused, while the for loop inside it was actually performing the correct check. I've made a patch to fix the issue: #95841

As for git blame on a shallow copy, iirc a shallow copy doesn't actually have the list of commits so it probably just returned the most recent commit.