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

Fix Issue 20644 - Invalid printf deprecation for ubyte passed to "%hhu" #10920

Merged
merged 1 commit into from Mar 28, 2020

Conversation

Luhrel
Copy link
Contributor

@Luhrel Luhrel commented Mar 14, 2020

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Luhrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20644 normal Invalid printf deprecation for ubyte passed to "%hhu"

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#10920"

@rainers
Copy link
Member

rainers commented Mar 16, 2020

It seems like the wrong fix if dlang/druntime#2988 is necessary. Please also add the test case from the bug report.

@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 16, 2020

@rainers why ?
Shouldn't bool be considered like a ubyte ?

It's not very clear what we should do with types which are promoted to int.
I see two possibilities:

  • Force the correct use of the format specifier.
  • Allow non-strict format specifier, so an ubyte can be passed to printf with the %d format specifier.

Edit: Just saw you comment in dlang/druntime#2988 - using the 2nd.

@Luhrel Luhrel force-pushed the fix20644 branch 5 times, most recently from a60cb5a to 88a7b4a Compare March 16, 2020 10:35
@rainers
Copy link
Member

rainers commented Mar 16, 2020

@rainers why ?

Because none of the C compilers complain about it.

Shouldn't bool be considered like a ubyte ?

yes, both should not emit a message with %d.

@WalterBright
Copy link
Member

WalterBright commented Mar 16, 2020

This is a regression from the merging of chkprintf.d with chkscanf.d. I suggest going over chkprintf.d and seeing why it worked correctly, as this fix doesn't look like that.

#10812

@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 17, 2020

@WalterBright That's because %hhd and %hd were considered like a %d in your implementation. (#10812, chkprintf.d: 127, 451).

Should we check for the correct use of these formats or simply bypass it like you did ?

@Luhrel Luhrel force-pushed the fix20644 branch 2 times, most recently from 65dae52 to ccfb7bd Compare March 25, 2020 08:47
@WalterBright
Copy link
Member

Should we check for the correct use of these formats or simply bypass it like you did ?

My implementation was pedantically correct according to the C Standard. If I made a mistake with that and am proven wrong, which is entirely possible, that should be a separate PR. But this particular behavior is not a mistake, because variadics always undergo the usual arithmetic promotions.

In general, refactorings must not include bug fixes and/or enhancements. I stress this again and again. Refactorings, bug fixes and enhancements should go in separate PRs. This regression slipped into a PR that was a bug fix and a refactoring and an enhancement, and it was done with no comment about including a bug fix.

I have a lot of experience with refactorings, bug fixes, and enhancements. Things go much better with much less time wasted chasing regressions when they are separate PRs.

test/runnable/test42.d Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants