Skip to content

Implement boolval() #60

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

Closed
wants to merge 3 commits into from
Closed

Implement boolval() #60

wants to merge 3 commits into from

Conversation

Jille
Copy link
Contributor

@Jille Jille commented Apr 18, 2012

Implementation based on strval()

@nikic
Copy link
Member

nikic commented Apr 18, 2012

How is this different from (bool) $var?

@Jille
Copy link
Contributor Author

Jille commented Apr 18, 2012

It isn't, but this is usable as callback.

@cataphract
Copy link
Contributor

@laruence That's not the same thing. For instance, it will fail if you pass an array, while convert_to_boolean works with any variable. @Jille 's implementation seems correct to me, though he could use convert_to_boolean_ex to avoid a separation if it's not necessary.

@Jille
Copy link
Contributor Author

Jille commented Apr 19, 2012

I accidentally also committed the convert_to_boolean -> convert_to_boolean_ex in the previous commit. I'm not sure whether that is the only change required.

@cataphract
Copy link
Contributor

@Jille Sorry, disregard that last part of my comment. convert_to_boolean_ex is not a good idea here (more cycles, though it would still work), because it precedes a copy anyway. Since return_value is only a zval * and not zval **, that copy is necessary.

@smalyshev
Copy link
Contributor

With anon functions, I'm not sure why function($x) { return (bool)$x;} doesn't really solve this use case.

@Jille
Copy link
Contributor Author

Jille commented Apr 19, 2012

Most of all it's a consistency improvement. We have strval, doubleval, floatval and intval so one would assume boolval also exists.

@Jille
Copy link
Contributor Author

Jille commented Apr 24, 2012

Should this be discussed on the internals list? Should I open a bug and hope someone will merge it? Should I just wait till someone hits the "Merge" button?

@nikic
Copy link
Member

nikic commented Apr 24, 2012

@Jille You should bring this up on internals. It adds a new function, so it can't be simply merged. Also it's debatable whether we really need it, so... writing a mail to internals should be the right thing to do :)

@dsp
Copy link
Member

dsp commented May 2, 2012

It makes sense to add it to 5.5

@dsp
Copy link
Member

dsp commented Jun 6, 2012

Add a test please and I will pull it. Also squash 90a3d47 and a5d113e together (if not I can do that myself)

@Jille
Copy link
Contributor Author

Jille commented Jun 6, 2012

74b4ef5 (in my boolval-branch) contains the function and a test. Thanks!

@dsp dsp closed this Jun 7, 2012
@dsp
Copy link
Member

dsp commented Jun 7, 2012

Merged as 753c336. Thank you for your contribution.

@dsp
Copy link
Member

dsp commented Jun 7, 2012

Would be great if you can also help the docteam adding that function to phpdoc :)

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.

6 participants