Skip to content

Add tests for sprintf and fprintf in fail_compilation/chkformat.d#10872

Closed
Luhrel wants to merge 2 commits intodlang:masterfrom
Luhrel:sfprintf-validation
Closed

Add tests for sprintf and fprintf in fail_compilation/chkformat.d#10872
Luhrel wants to merge 2 commits intodlang:masterfrom
Luhrel:sfprintf-validation

Conversation

@Luhrel
Copy link
Contributor

@Luhrel Luhrel commented Mar 6, 2020

Blocked by dlang/druntime#2972

@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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#10872"

@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 7, 2020

Can now be merged.

@thewilsonator
Copy link
Contributor

dhry.d(629): Deprecation: format specifier `"%7.1lf"` is invalid
dhry.d(631): Deprecation: format specifier `"%10.1lf"` is invalid
dhry.d(632): Deprecation: format specifier `"%10.3lf"` is invalid
dhry.d(637): Deprecation: width argument `& Reg_Define` for format specification `"%*s"` must be `int`, not `char*`
dhry.d(637): Deprecation: more format specifiers than 1 arguments
Error: unknown, please file report on issues.dlang.org
dhry.d(638): Deprecation: format specifier `"%7.1lf"` is invalid
dhry.d(639): Deprecation: format specifier `"%10.1lf"` is invalid
dhry.d(640): Deprecation: format specifier `"%10.3lf"` is invalid

@MoonlightSentinel
Copy link
Contributor

That's already present in master, see https://issues.dlang.org/show_bug.cgi?id=20645

@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 7, 2020

dhry.d(629): Deprecation: format specifier `"%7.1lf"` is invalid
dhry.d(631): Deprecation: format specifier `"%10.1lf"` is invalid
dhry.d(632): Deprecation: format specifier `"%10.3lf"` is invalid
dhry.d(637): Deprecation: width argument `& Reg_Define` for format specification `"%*s"` must be `int`, not `char*`
dhry.d(637): Deprecation: more format specifiers than 1 arguments
Error: unknown, please file report on issues.dlang.org
dhry.d(638): Deprecation: format specifier `"%7.1lf"` is invalid
dhry.d(639): Deprecation: format specifier `"%10.1lf"` is invalid
dhry.d(640): Deprecation: format specifier `"%10.3lf"` is invalid

Somehow, I don't get any error on my system:

$ test/run.d runnable/dhry.d
unit_test_runner is already up-to-date
d_do_test is already up-to-date
sanitize_json is already up-to-date
dshell_prebuilt is already up-to-date
 ... runnable/dhry.d                -release -O -g -inline  -fPIC ()

Which command did you run ?

@Luhrel Luhrel force-pushed the sfprintf-validation branch 2 times, most recently from 335da86 to e29b7ff Compare March 8, 2020 09:36
@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 8, 2020

Can someone re-run the auto-tester and buildkite ?
It should be ready to merge.

@MoonlightSentinel
Copy link
Contributor

Please give this a force push to trigger the auto-tester

@thewilsonator
Copy link
Contributor

@MoonlightSentinel You can restart individual auto-tees by clicking on the Run ID column, and then clicking on deprecate on the Deleted row. Alternatively when all of the tests are delete (on the main screen) just add auto-merge and it will prioritise that PR.

@MoonlightSentinel
Copy link
Contributor

Thanks @thewilsonator but I don't seem to have the permission to restart single jobs (there is no deprecate button for me).

@thewilsonator
Copy link
Contributor

You nee to log in, see the top left corner.

@MoonlightSentinel
Copy link
Contributor

Nevermind, I was already logged in but only looked at already deleted runs.

@thewilsonator
Copy link
Contributor

std\math.d(246): Deprecation: argument `cast(double)x` for format specification `"%.*Lg"` must be `real`, not `double`
std\math.d(247): Deprecation: argument `cast(double)y` for format specification `"%.*Lg"` must be `real`, not `double`

@WalterBright
Copy link
Member

Allow double type for Lg format specifier

PLEASE follow the SPECIFICATION, do not make up rules. C99 7.19.6.1.7 says "L Specifies that a following a, A, e, E, f, F, g, or G conversion specifier applies to a long double argument."

Not a double. A long double, i.e. real.

Please also defer to the actual Standard, not some web site that rewrote it (to avoid copyright infringement concerns) and got it wrong.

Refactorings must not change behavior, either.

@Luhrel
Copy link
Contributor Author

Luhrel commented Apr 9, 2020

Allow double type for Lg format specifier

PLEASE follow the SPECIFICATION, do not make up rules. C99 7.19.6.1.7 says "L Specifies that a following a, A, e, E, f, F, g, or G conversion specifier applies to a long double argument."

Not a double. A long double, i.e. real.

Please also defer to the actual Standard, not some web site that rewrote it (to avoid copyright infringement concerns) and got it wrong.

Refactorings must not change behavior, either.

In your changelog, you said:

For printf, it takes a generous, rather than strict, view of compatiblity.

@WalterBright
Copy link
Member

Unfortunately, there is no way a 10 byte real is binary compatible with an 8 byte double.

Besides, and I cannot over-emphasize this, refactorings MUST NOT INCLUDE BUG FIXES OR BEHAVIOR CHANGES.

@Luhrel Luhrel force-pushed the sfprintf-validation branch from d3b3668 to ab6ff63 Compare April 9, 2020 11:34
@Luhrel Luhrel force-pushed the sfprintf-validation branch from ab6ff63 to 735be9d Compare April 18, 2020 20:15
@Luhrel Luhrel requested a review from WalterBright April 18, 2020 20:25
@Luhrel Luhrel force-pushed the sfprintf-validation branch from 735be9d to 4f1880f Compare May 5, 2020 22:33
@MoonlightSentinel
Copy link
Contributor

Is this still useful given pragma(printf)?

@Luhrel Luhrel changed the title Add format validation for sprintf and fprintf Add tests for sprintf and fprintf in fail_compilation/chkformat.d Jul 2, 2020
@Luhrel
Copy link
Contributor Author

Luhrel commented Jul 2, 2020

Is this still useful given pragma(printf)?

As Walter did all the work, this PR is very trivial now.

I close it. Feel free to reopen it if you really want to.

@Luhrel Luhrel closed this Jul 2, 2020
@Luhrel Luhrel deleted the sfprintf-validation branch July 9, 2020 09:41
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.

5 participants