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

Use "defined" in DBM::Deep::Array::PUSH #1

Merged
merged 3 commits into from
Jun 30, 2013
Merged

Conversation

jes
Copy link
Contributor

@jes jes commented May 18, 2013

This way it is possible to push 0's and empty strings.

https://rt.cpan.org/Public/Bug/Display.html?id=85414

This way it is possible to push 0's and empty strings.
@jes
Copy link
Contributor Author

jes commented May 18, 2013

Note this still doesn't fix this code entirely.

Consider:

push @a, undef, "hello";

With normal arrays this works fine, but here it will break the loop before pushing "hello". I'm not sure what to do about this.

The previous commit would exit the loop upon an undef element, whereas this version always iterates over all of the arguments.
@jes
Copy link
Contributor Author

jes commented May 18, 2013

Should be sorted now.

@robkinyon
Copy link
Owner

Jes -

What do you think about switching the code to something like:

while ( length @_ ) {
    $self->STORE( $length, shift(@_) );
    $length++;
}

That way, we don't care what the thing is at all. We just care that a thing was passed in, whatever it is.

If you think that works, please amend the patch and (most importantly!) add tests. All the test cases you've suggested are good ideas. :)

And, finally, please add yourself to the CONTRIBUTORS list in the DBM/Deep.pod file.

@jes
Copy link
Contributor Author

jes commented May 19, 2013

That won't work:

$ perl -e '@a = (); print length(@a), "\n"'
1

The length will never get to 0. For now I'll just add the test and CONTRIBUTORS line. Feel free to change the loop to

while ( @_ ) {
    $self->STORE( $length, shift(@_) );
    $length++;
}

Personally, I think the for loop version is nicer.

@jes
Copy link
Contributor Author

jes commented May 27, 2013

I think this is suitable for merging now, unless there is anything else you would like me to change?

@jes
Copy link
Contributor Author

jes commented Jun 4, 2013

Anything I could do to help?

robkinyon added a commit that referenced this pull request Jun 30, 2013
Use "defined" in DBM::Deep::Array::PUSH
@robkinyon robkinyon merged commit 5584c43 into robkinyon:master Jun 30, 2013
@robkinyon
Copy link
Owner

Merged and released. Thanks!

On Tue, Jun 4, 2013 at 9:05 AM, James Stanley [email protected]:

Anything I could do to help?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-18919630
.

Thanks,
Rob Kinyon

@jes
Copy link
Contributor Author

jes commented Jul 2, 2013

Cheers Rob :)

@cpansprout
Copy link
Collaborator

Thank you, Rob, for taking care of this and other issues. I just have not had time lately to do anything with DBM::Deep.

@robkinyon
Copy link
Owner

No worries. That's why there's two of us.

On Fri, Jul 19, 2013 at 12:48 PM, cpansprout [email protected]:

Thank you, Rob, for taking care of this and other issues. I just have not
had time lately to do anything with DBM::Deep.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-21273009
.

Thanks,
Rob Kinyon

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.

3 participants