Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Printf: Fix printf hex alternate zero #5811

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

spineki
Copy link
Contributor

@spineki spineki commented Jan 8, 2024

Summary

  • Fixed case where giving only 0 would crash printf due to an attempt to read it as an octal
  • Fixed wrong prefix for lower and upper hex alternative format. Now, the "0x" is not shown when the value is 0.

Code

GNU:
> printf %#x 0
0

uutils: fails
now shows 0

Fixed an

Related issues:

original : #5810
part of other changes (PR): #5794
part of other changes: #5709
octal similar problem: #5807

Copy link

github-actions bot commented Jan 8, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@samueltardieu
Copy link
Contributor

I would prefer this PR not to be merged as-is if the larger #5783 PR is merged, as it rewrites the parser used in printf and it doesn't have of the single 0 interpreted as octal already.

@tertsdiepraam
Copy link
Member

@samueltardieu I agree. For clarity, you're suggesting to only keep the 0x fix from this, right?

@samueltardieu
Copy link
Contributor

@samueltardieu I agree. For clarity, you're suggesting to only keep the 0x fix from this, right?

Yes, and the associated tests.

@spineki
Copy link
Contributor Author

spineki commented Jan 9, 2024

I did the octal fix in the first place because printf would fail before reaching the 0x-prefix fix.
Thus, amongst the four new tests, those named xxxx_0 will fail without the octal fix, even if the 0x prefix fix is kept.

It means that if you want to keep a subset of this PR without the octal fix, you can

  • keep tests on lower hex and upper hex (not the 0 variant), but the prefix fix for 0 will not be tested.
  • keep the 4 tests, but the xxx_0 tests variants will fail.

@tertsdiepraam
Copy link
Member

Maybe we could try to merge @samueltardieu's PR first and then merge this. That should make everything alright, shouldn't it?

@tertsdiepraam tertsdiepraam force-pushed the fix-printf-hex-alternate-zero branch from 790bfa2 to 0648321 Compare January 10, 2024 15:56
@tertsdiepraam
Copy link
Member

I've rebased this on top of main after merging #5783.

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@tertsdiepraam tertsdiepraam merged commit 3bd9f0e into uutils:main Jan 17, 2024
@spineki spineki deleted the fix-printf-hex-alternate-zero branch January 17, 2024 17:11
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