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

Fixed bug in DTEDRasterReader where AVList was assumed non-null #7

Closed
wants to merge 4 commits into from
Closed

Conversation

mikeoertli
Copy link

In DTEDRasterReader added check for null AVList before trying to use it, if null create a new empty AVListImpl.

…it, if null create a new empty AVListImpl.
@zglueck
Copy link
Contributor

zglueck commented Jul 21, 2016

I was wondering if you could provide an example or use case of an instance where a null AVList would be provided by the caller? While checking for a null AVList does avoid a potential null pointer exception, the newly created AVList drops out of scope on DTEDRasterReader return. If you could provide some additional details or an example of the situation we can work from there on a solution. Thanks!

@mikeoertli
Copy link
Author

I don't have an example where it is valid or desirable necessarily, but for RaptorX (raptorx.org) there was a case where we were calling into the logic with "null" for some reason. I have fixed it on our end, but I thought this might be an easy robustness improvement.

@zglueck
Copy link
Contributor

zglueck commented Jul 21, 2016

I was talking with @pdavidc about this and he pointed out there is a null check for setting the params to the AVList on line 61 of the current method. So it does seem a null check is missing at the point you identified. If you are up for it, I'll merge the request if you add a null check before the first call to the params.setValue method.

With the params null check on line 61 and, any new instantiation of AVList dropping out of scope at the end of the method, I don't think instantiating a new AVList is required, what about you?

Let me know if you have any questions or if I missed something. Please match the formatting of the existing code to include the bracket positioning and order of null check if statements.

Thanks @mikeoertli for the catch! We appreciate your effort to make WWJ better!

@mikeoertli
Copy link
Author

I've got a big push for a project through 8/4, should be able to address this shortly after. At a glance, I agree with what you've outlined, thanks for the feedback.

@zglueck
Copy link
Contributor

zglueck commented Aug 15, 2016

Thanks @mikeoertli !

@mikeoertli
Copy link
Author

Sorry for the whole pile of commits there, was having issues with my git client and forgot to clean that up before pushing. I think everything we'd talked about is in there, but if I Mondayed it up, let me know :).

@zglueck
Copy link
Contributor

zglueck commented Aug 18, 2016

@mikeoertli, wanted to thank you again for identifying and working with us on a solution to this issue. One of the motivations for moving to Github was facilitation of community feedback and coordination. I'm modifying your approach slightly and pushing changes this evening. Please feel free to reach out in the future with other issues you find!

@zglueck zglueck closed this Aug 18, 2016
@basisbit
Copy link

PS: in open source development it usually is better to accept such a pull request and then commit code changes to it to have a correct changelog and attribution of who did which changes and for what reason. Otherwise you will loose new developers from the community quickly.

@pdavidc pdavidc added the bug label Dec 9, 2016
@pdavidc pdavidc added this to the v2.1.0 milestone Dec 9, 2016
@wcmatthysen wcmatthysen mentioned this pull request Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants