FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

BUG: sscanf did not parse arguments in _tkagg.cpp by mattip · Pull Request #10486 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

BUG: sscanf did not parse arguments in _tkagg.cpp - #10486

Merged
QuLogic merged 3 commits into
matplotlib:masterfrom
mattip:issue10479
Feb 17, 2018
ConversationCommits (3)ChecksFiles changed
Merged

BUG: sscanf did not parse arguments in _tkagg.cpp#10486
QuLogic merged 3 commits into
matplotlib:masterfrom
mattip:issue10479

Conversation

mattip commented Feb 15, 2018

Copy link
Copy Markdown
Contributor

PR Summary

Fix issue #10479 which was caused by a quirk (bug?) in the way sscanf works on MSVC (windows). It seems that additional values cannot be used after "%Iu" which scans into size_t

Also added line drawing in an interactive test

QuLogic commented Feb 16, 2018

Copy link
Copy Markdown
Member

Does anyone know why we need to route through tk.call instead of the Python-C API? Then we wouldn't have to bother with this string parsing to transfer pointers.

tacaswell commented Feb 16, 2018

Copy link
Copy Markdown
Member

Thanks for the quick fix @mattip !

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 16, 2018
tacaswell modified the milestones: v2.2.1, v2.2.0 Feb 16, 2018

tacaswell commented Feb 16, 2018

Copy link
Copy Markdown
Member

@QuLogic My guess is that answer is lost to the sands of time and changing it is probably a bigger change than we want to fit in for 2.2.

WeatherGod reviewed Feb 16, 2018
View reviewed changes

WeatherGod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, this is in the blitting code, which doesn't get a lot of attention. There has always been some weirdness with the image buffer and Tk because Tk does not provide a python API for passing it around.

Is this the only place where this is done? Have we looked at what is done elsewhere for the image buffer?

Comment thread src/_tkagg.cpp Outdated
@@ -74,9 +74,9 @@ static int PyAggImagePhoto(ClientData clientdata, Tcl_Interp *interp, int
return TCL_ERROR;
}
/* get buffer from str which is "ptr height width" */

WeatherGod Feb 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

you'll want to update the comment

mattip Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

fixed in 816ffba

mattip commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

@tacaswell sorry for not properly testing

QuLogic commented Feb 17, 2018

Copy link
Copy Markdown
Member

@ImportanceOfBeingErnest are you able to test this?

ImportanceOfBeingErnest commented Feb 17, 2018

Copy link
Copy Markdown
Member

@QuLogic If testing means building matplotlib from source, I'm affraid I can't do that currently.

QuLogic merged commit af4830e into matplotlib:master Feb 17, 2018

QuLogic commented Feb 17, 2018

Copy link
Copy Markdown
Member

We can leave it for the next rc.

lumberbot-app Bot pushed a commit that referenced this pull request Feb 17, 2018
Backport PR #10486: BUG: sscanf did not parse arguments in _tkagg.cpp
6d11782
lumberbot-app Bot mentioned this pull request Feb 17, 2018
Merged

afvincent commented Feb 17, 2018

Copy link
Copy Markdown
Contributor

@QuLogic : (Could you) Run the test suite, @ImportanceOfBeingErnest .
@ImportanceOfBeingErnest : I’m sorry, @QuLogic . I’m afraid I can’t do that (currently).

(Sorry, I could not resist 🐑 . The exact reference is of course this one)

QuLogic mentioned this pull request Feb 17, 2018
Closed
dstansby added a commit that referenced this pull request Feb 17, 2018
Merge pull request #10506 from matplotlib/auto-backport-of-pr-10486
652a4c3 
Backport PR #10486 on branch v2.2.x
anntzer mentioned this pull request Mar 5, 2018
Merged
6 tasks
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

WeatherGod WeatherGod left review comments
tacaswell tacaswell approved these changes

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v2.2.0

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL