Skip to content

fix the bug in PR #21620 correctly #22282

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
Merged
merged 2 commits into from
Jun 10, 2017
Merged

fix the bug in PR #21620 correctly #22282

merged 2 commits into from
Jun 10, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jun 8, 2017 •
edited
Loading

fix #22256

@vtjnash vtjnash added backport pending 0.6 bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code labels Jun 8, 2017
@ararslan ararslan requested a review from JeffBezanson June 8, 2017 01:18
@tkelman
Copy link
Contributor

tkelman commented Jun 8, 2017

would be good if the commit message briefly described what either the bug or the fix was

@davidanthoff davidanthoff mentioned this pull request Jun 8, 2017
Closed
@galenlynch
Copy link
Contributor

Looks like this may still have some issues: See this comment in Feather.jl #46

@vtjnash
Copy link
Member Author

vtjnash commented Jun 9, 2017

@tkelman could you please suggest a new commit message rather than just asking for rework. In general, I strongly agree that commit messages should be more descriptive of the bug than an issue number. In this case, I just didn't have much more to say.

@tkelman
Copy link
Contributor

tkelman commented Jun 9, 2017

What was the bug? Uninitialized memory of some kind, going by the reduction of #22256? I didn't author the diff here, I don't know enough about what was wrong or what these lines of code are supposed to be doing vs what they were doing to come up with a summary for you - I trust you're capable of doing so if you try to.

@JeffBezanson
Copy link
Member

Something like "fix bug in setfield! codegen with abstract type"?

@vtjnash
implement setfield! case correctly of PR to optimize field access whe…
6ab0f0a 
…n field types don't depend on parameters

fixes the bug in PR #21620 correctly
@vtjnash
Copy link
Member Author

vtjnash commented Jun 9, 2017

The bug is that PR #21620 would have caused Julia to crash if it had included a test. Somewhat surprising how little this code path gets used!

@vtjnash vtjnash merged commit 9b8c608 into master Jun 10, 2017
@vtjnash vtjnash deleted the jn/22256 branch June 10, 2017 15:01
tkelman pushed a commit that referenced this pull request Jun 17, 2017
@vtjnash @tkelman
implement setfield! case correctly of PR to optimize field access whe…
82c25d9 
…n field types don't depend on parameters

fixes the bug in PR #21620 correctly

(cherry picked from commit 6ab0f0a)
ref #22282

fix test case to use 4 space indent instead of 3
(cherry picked from commit ff2ad4e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffBezanson JeffBezanson JeffBezanson approved these changes

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sporadic segfaults in v0.6.0-rc2 using ODBC package
4 participants
@vtjnash @tkelman @galenlynch @JeffBezanson