numfmt: name the only conversion in a bad --format directive - #14001
Merged
Conversation
Only --format and --field pointed a caret at what they refused. Carry the option name and the value as typed out of parsing, so --from, --to, --from-unit, --to-unit, --padding and --header underline their value on the command line, and so does a number operand that fails to convert. Each gets a help line naming what the option takes, and a label where the help is not enough: auto being a unit only --from can use, or a zero that parses fine and fails on meaning. A bad --format directive gains one too, naming the single conversion numfmt has, since %d, %e and friends are commonly tried there -- but only where one was typed: "%10" has none to name. Placing the caret on an operand also fixes a panic on a whitespace-only one, whose offsets crossed when sliced; puts it on the operand rather than on the detached value of an earlier option holding the same text, as in `--suffix q --from si q`; and drops the suffix advice for input with no number at all.
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | numfmt_stream_to_si_precision |
128.5 KB | 140.5 KB | -8.55% |
| ❌ | Simulation | numfmt_large_numbers_si[10000] |
93.2 ms | 96.7 ms | -3.71% |
| ⚡ | Simulation | numfmt_stream_to_si_precision |
390.9 ms | 346.4 ms | +12.86% |
| ⚡ | Simulation | numfmt_stream_to_si |
431.1 ms | 414.1 ms | +4.11% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sylvestre:numfmt (0a3a7ba) with main (bfd7c63)
Footnotes
-
50 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An invalid --format directive pointed at a single character, and said nothing about what numfmt would have accepted there. Label the caret with the one conversion numfmt has, since %d, %e and the other C conversions are commonly tried there -- but only where one was actually typed: a format that ends before its conversion, as in "%10", has none to name, and only the character standing in for the f is at fault, so a valid suffix after it stays out of the underline.
The caret a refused number gets is placed the same way, and now:
--suffix q --from si q;A unit size loses the "must be at least 1" label when an unknown suffix, not the zero, is what it got wrong.