Skip to content

libpcp_web: accept NaN/Inf in series_pmAtomValue_conv_str on float/double#2602

Merged
kurik merged 1 commit into
performancecopilot:mainfrom
kurik:s390x
May 28, 2026
Merged

libpcp_web: accept NaN/Inf in series_pmAtomValue_conv_str on float/double#2602
kurik merged 1 commit into
performancecopilot:mainfrom
kurik:s390x

Conversation

@kurik
Copy link
Copy Markdown
Contributor

@kurik kurik commented May 28, 2026

The first-character digit check rejected pmAtomStr_r() output like "nan" from 0.0/0.0 on s390x (positive quiet NaN), breaking pmseries division tests 1886 and 1906.

For some reason the FPU on s390x returns NaN with sign bit clear which is different from most of the other architectures which return negative NaN (sign bit set). Both ways are allowed by IEEE 754 which states only that Nan should be returned (no sign defined).

Edit: I can reproduce this on ppc64le arch as well, so it is not specific for s390x FPUs.

…uble

The first-character digit check rejected pmAtomStr_r() output like "nan"
from 0.0/0.0 on s390x (positive quiet NaN), breaking pmseries division
tests 1886 and 1906.

For some reason the FPU on s390x returns NaN with sign bit clear which
is different from most of the other architectures which return negative
NaN (sign bit set). Both ways are allowed by IEEE 754 which states only
that Nan should be returned (no sign defined).
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The PR strengthens numeric validation in the series_pmAtomValue_conv_str() function by adopting a safer unsigned char cast in the isdigit() check for float and double conversions, improving the robustness of string-to-atom value conversion while maintaining sign prefix handling.

Changes

String Conversion Validation

Layer / File(s) Summary
Float/Double conversion string validation
src/libpcp_web/src/query.c
The PM_TYPE_FLOAT/PM_TYPE_DOUBLE branch now assigns the pmAtomStr_r() result and validates the string prefix with isdigit((unsigned char)*s) instead of isdigit(*s), enforcing stricter type safety while still permitting leading +/- characters.

A digit check so fine, 🐰
Unsigned char cast, designs align,
No more sign confusion,
Just numeric precision,
Atoms convert by design.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: accepting NaN/Inf in the float/double conversion path of series_pmAtomValue_conv_str function.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the issue being fixed: rejecting NaN output from pmAtomStr_r() on s390x, and provides technical context about IEEE 754 compliance and architectural differences.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@natoscott
Copy link
Copy Markdown
Member

@kurik LGTM.

@kurik kurik merged commit 16e17fa into performancecopilot:main May 28, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants