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.

Sign up for GitHub

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

Jump to bottom

add validation of printf format and arguments #10812

Merged
merged 1 commit into from
Feb 25, 2020
Merged

add validation of printf format and arguments #10812

merged 1 commit into from
Feb 25, 2020

Conversation

WalterBright
Copy link
Member

This wasn't that hard to do, so I just did it.

I know it needs a changelog.

It currently only works for printf, but I'll extend to any function with printf formatting after this is incorporated.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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 fetch digger
dub run digger -- build "master + dmd#10812"

@WalterBright WalterBright force-pushed the chkprintf branch 5 times, most recently from 30b2d21 to 920d3c4 Compare February 21, 2020 10:05
@WalterBright
Copy link
Member Author

Looks like it's already finding bugs in DMD itself. I'm so ashamed :-/

@WalterBright WalterBright mentioned this pull request Feb 21, 2020
Merged
@WalterBright
Copy link
Member Author

blocked by #10816

@WalterBright WalterBright force-pushed the chkprintf branch 16 times, most recently from 9c274ff to 42fda3c Compare February 22, 2020 01:53
@Geod24
Copy link
Member

Geod24 commented Feb 24, 2020

Also make sure you made an annotated tag (git tag -a). If not you can re-do it with git tag -a -f v1.1.1.

@WalterBright
Copy link
Member Author

@Geod24 that worked, thanks! And finally, I understand the difference between upstream and origin.

@Geod24
Copy link
Member

Geod24 commented Feb 24, 2020

Great! If you have some time, I highly recommend this online book. Chapters 1 to 3, 7, 8 and 10 give invaluable informations.

Personally, it turned what I saw as an over-complicated and obscure program I had to deal with to contribute into a powerful tool I now use for every project.

Also, I've restarted your Buildkite job.

@WalterBright
Copy link
Member Author

I've restarted your Buildkite job.

Thanks, I couldn't figure out a way to do that without redoing all the tests. BTW, did you check the sociomantic buildkite libraries?

@Geod24
Copy link
Member

Geod24 commented Feb 24, 2020

BTW, did you check the sociomantic buildkite libraries?

I took a (very quick) peek. It looks like the code that's failing was imported from Phobos and the fixes should be the same as what you did in Phobos.

Thanks, I couldn't figure out a way to do that without redoing all the tests.

At the top, when you have the list of jobs, just click on the job that's failing and it'll bring you to a collapsed tab where you have the full log.
There is a retry button next to it:
Screen Shot 2020-02-24 at 12 42 56

@WalterBright
Copy link
Member Author

I don't have privileges for Sociomantic libraries, so I can't fix it.

@Geod24
Copy link
Member

Geod24 commented Feb 24, 2020

The example above was just using Ocean as an example because Undead was already retriggered and Ocean is the only one still failing (swarm as well but because it depends on Ocean).

And for the fixes, they take pull requests. But even then, making release is more involved.
Note that if it is a deprecation, it is not blocking you anymore.

wilzbach
wilzbach previously requested changes Feb 24, 2020
View reviewed changes
Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Needs to be a deprecation as we formally agreed to no longer ship breaking releases.

@WalterBright
Copy link
Member Author

You're right, and I was planning to make it a -preview switch after we got as much of our stuff fixed as we can. The only thing left is the ocean library. @Geod24 how about taking care of it?

@wilzbach
Copy link
Member

It doesn't need to be hidden behind a preview switch (those get never found nor activated), it just needs to a non-breaking deprecation.

@WalterBright
Copy link
Member Author

Good idea, deprecation it is.

@WalterBright
Copy link
Member Author

@Geod24 : There is a retry button next to it:

Nope. Must be something I'm not privileged to access.

@WalterBright
Copy link
Member Author

@wilzbach This PR is now ready to go. The buildkite/dmd bugs look unrelated.

Geod24
Geod24 reviewed Feb 24, 2020
View reviewed changes
changelog/chkprintf.md Outdated
@@ -0,0 +1,51 @@
Check that arguments to a printf format string are compatible
with that string. Issue errors for incompatibilities.
Copy link
Member

Choose a reason for hiding this comment

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

This is a title and so two sentences are far too long.

Also it doesn't show up in preview. @wilzbach do we support md file ? Our documentation only says .dd.

Copy link
Member Author

Choose a reason for hiding this comment

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

urgh, it'll take a couple hours to go all green again if I modify the changelog :-(

Copy link
Member

Choose a reason for hiding this comment

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

Yeah while DMD now supports markdown, the changelog script hasn't been updated yet.

Geod24
Geod24 reviewed Feb 24, 2020
View reviewed changes
changelog/chkprintf.md

No attempt is made to fix the arguments or the format string.

In order to use non-Standard printf formats, an easy workaround is:
Copy link
Member

Choose a reason for hiding this comment

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

non-Standard, why the uppercase ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I am referring to the C99 Standard, not just any standard.

@Geod24
Copy link
Member

Geod24 commented Feb 24, 2020

Restarted the failing Buildkite tests. As mentioned, could you split this into two commits, this way if the printf checks are problematic we can just revert this commit and not touch the examples (this is very easy to do via command line).

@WalterBright WalterBright mentioned this pull request Feb 24, 2020
Merged
@WalterBright
Copy link
Member Author

Reverting this PR is pretty much just deleting chkprintf.d. It's less work than trying to muck this into two commits.

@WalterBright
Copy link
Member Author

spec pull: dlang/dlang.org#2760

@WalterBright WalterBright merged commit bd6346e into dlang:master Feb 25, 2020
@WalterBright WalterBright deleted the chkprintf branch February 25, 2020 04:21
@WalterBright WalterBright mentioned this pull request Feb 29, 2020
Merged
@Luhrel Luhrel mentioned this pull request Mar 1, 2020
Merged
@WalterBright WalterBright mentioned this pull request Mar 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilzbach wilzbach wilzbach left review comments

@Geod24 Geod24 Geod24 left review comments

@RazvanN7 RazvanN7 RazvanN7 approved these changes

Assignees
No one assigned
Labels
Severity:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@WalterBright @dlang-bot @rainers @Geod24 @adamdruppe @wilzbach @RazvanN7