Skip to content

interpreter: convert strings to numbers by their leading numeric prefix - #142

Open
jayhemnani9910 wants to merge 2 commits into
uutils:mainfrom
jayhemnani9910:fix/string-to-number-prefix
Open

jayhemnani9910 wants to merge 2 commits into
uutils:mainfrom
jayhemnani9910:fix/string-to-number-prefix

Conversation

@jayhemnani9910

Copy link
Copy Markdown
Contributor

A string turns into a number only when the whole string is a valid Rust float, so blanks around a number or anything after it give 0. gawk 5.3.2 vs main:

$ printf 'a, 12 \nb,  4\n' | awk 'BEGIN { FS = "," } { s += $2 } END { print s }'
gawk: 16
main: 0

$ awk 'BEGIN { print "3x" + 0, int("4.9x"), "inf" + 0 }'
gawk: 3 4 0
main: 0 0 inf

to_num for a string now skips leading blanks and reads the longest decimal prefix (sign, digits, ., exponent only when digits follow it), like strtod without hex. inf and nan count only with a sign and nothing but blanks after them, which is what gawk does; before, Rust's parser turned a bare inf, nan or infinity into a number.

New tests string_to_number_uses_leading_numeric_prefix and fields_with_blanks_around_numbers_sum fail without the change. cargo test --workspace: 212 passed (210 on main plus the two), clippy and fmt clean. 38 strings (blanks incl. \v\f\r, 5., .5, 1e, 1e+, 1.2.3, 0x1A, +INF, +infinity, 1e400, ...) give the same value as gawk; inf and nan still print differently (inf vs +inf), which is output formatting, not this change.

Copilot AI balanced review requested due to automatic review settings October 24, 2026 07:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@github-actions

Copy link
Copy Markdown

GNU awk testsuite comparison:

Test results comparison:
  Current:   TOTAL: 666 / PASSED: 101 / FAILED: 467 / SKIPPED: 98
  Reference: TOTAL: 666 / PASSED: 100 / FAILED: 468 / SKIPPED: 98

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + hex

Comment thread interpreter/src/vm/types.rs Outdated
/// are skipped and the longest prefix that is a decimal number counts, so
/// `" 12 "` is 12, `"3x"` is 3 and `"x"` is 0. Like gawk, `inf` and `nan`
/// are only recognized with a sign and nothing else but blanks around them.
fn str_to_num(s: &[u8]) -> f64 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strtonum in builtins.rs still falls back to s.parse(), so strtonum("3x") gives 0 (gawk gives 3)
could it use this function too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in ae2eb12. strtonum now falls back to str_to_num, and the hex and octal branches also read just the leading digits, as gawk does: strtonum("0x1Fz") is 31, strtonum("017x") is 15, and an 8, 9, . or exponent after the leading 0 makes it decimal ("0778" is 778, "01.5" is 1.5). Like gawk, the prefix only counts at the very start, so " 0x10" and "+0x10" are 0. The new test is checked against gawk 5.3.2.

.ok()
.and_then(|s| s.parse().ok())
.unwrap_or(0.),
Self::String(s) => str_to_num(s),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about Str and StrNum? they still hit _ => 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in ae2eb12: Str goes through str_to_num, and StrNum returns its stored num. Nothing builds a StrNum yet, so only Str has a test.

@Alonely0

Copy link
Copy Markdown
Collaborator

sorry, needs to be rebased!

@Alonely0
Alonely0 force-pushed the fix/string-to-number-prefix branch from 952c091 to 9ee59e1 Compare October 24, 2026 16:46
Copilot AI balanced review requested due to automatic review settings October 24, 2026 16:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@github-actions

Copy link
Copy Markdown

GNU awk testsuite comparison:

Test results comparison:
  Current:   TOTAL: 666 / PASSED: 101 / FAILED: 467 / SKIPPED: 98
  Reference: TOTAL: 666 / PASSED: 100 / FAILED: 468 / SKIPPED: 98

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + hex

Copilot AI balanced review requested due to automatic review settings October 24, 2026 18:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@github-actions

Copy link
Copy Markdown

GNU awk testsuite comparison:

Test results comparison:
  Current:   TOTAL: 666 / PASSED: 101 / FAILED: 467 / SKIPPED: 98
  Reference: TOTAL: 666 / PASSED: 100 / FAILED: 468 / SKIPPED: 98

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + hex

@jayhemnani9910
jayhemnani9910 force-pushed the fix/string-to-number-prefix branch from ae2eb12 to 52b26e2 Compare October 25, 2026 10:24
Copilot AI balanced review requested due to automatic review settings October 25, 2026 10:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@github-actions

Copy link
Copy Markdown

GNU awk testsuite comparison:

Test results comparison:
  Current:   TOTAL: 666 / PASSED: 101 / FAILED: 467 / SKIPPED: 98
  Reference: TOTAL: 666 / PASSED: 100 / FAILED: 468 / SKIPPED: 98

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + hex

@Alonely0

Copy link
Copy Markdown
Collaborator

I don't think it's necessary to re-implement the whole numeric parser, but let me take a look at the GNU docs and then review the code.

@jayhemnani9910
jayhemnani9910 force-pushed the fix/string-to-number-prefix branch from 52b26e2 to d4a2c32 Compare October 25, 2026 14:45
Copilot AI balanced review requested due to automatic review settings October 25, 2026 14:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@github-actions

Copy link
Copy Markdown

GNU awk testsuite comparison:

Test results comparison:
  Current:   TOTAL: 666 / PASSED: 102 / FAILED: 466 / SKIPPED: 98
  Reference: TOTAL: 666 / PASSED: 101 / FAILED: 467 / SKIPPED: 98

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + hex

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.

4 participants