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

Feature/log2 #337

Closed
wants to merge 4 commits into from
Closed

Feature/log2 #337

wants to merge 4 commits into from

Conversation

marc-mabe
Copy link
Contributor

This PR adds the function log2. It works the same as the C function and is an addition to the always translated C functions to PHP like log1p and log10

It also fixes an issue on 32bit environments calculating the second logarithm as an integer:

var_dump(log2(64), (int)log2(64)); // float(6) & int(6)
var_dump(log(64, 2), (int)log(64, 2)); // float(6) & int(5)

The method log2 is part of the C99 standard - so there is no HAVE_LOG2 check done.
(Not tested if some environments doesn't support it)

@yohgaki
Copy link
Contributor

yohgaki commented Jul 18, 2013

Having basic math functions is good. IMO.
I think configure test is not required for log2().
Comments?

@dsp
Copy link
Member

dsp commented Sep 16, 2013

Thanks for this pull request. We usually try to avoid adding new global functions to the core. We wan't to avoid clashes with existing functions and avoid maintenance overhead. In this case there is a log(X, 2) equivalent to log2 so it would be just an abbrev that saves you 3 chars, which is not sufficient to have possible BC breaks in existing code bases. Imagine people using:

function log2($x) {
   return log($x, 2);
}

in their code. The new function would cause a fatal error for close to no gain. Therefore I am closing this PR.

@dsp dsp closed this Sep 16, 2013
@marc-mabe
Copy link
Contributor Author

@dsp Dit you read the example of the logarithm of 2 issue (not bug) on 32bit platforms using the method log()

var_dump(log2(64), (int)log2(64)); // float(6) & int(6)
var_dump(log(64, 2), (int)log(64, 2)); // float(6) & int(5)

That's the reason I added it - not saving of 2 characters.

@dsp
Copy link
Member

dsp commented Sep 16, 2013

@marc-mabe The example is a rounding error in floating point calculation. It's a standard problem when casting floats to int, therefore you cannot rely on the conversation (int)x==(float)y. Therefore I don't think this is a bug, but rather just happens due to how floating point works. Also if it one might consider it being a bug, we want to start fixing it in log(). For example check if the second argument in log() is 2 and use log2() internally instead to be less prone to floating point issues.

@marc-mabe
Copy link
Contributor Author

@dsp As noted above it's not a bug, it's an issue / problem. Sure it's coming from rounding errors but log2 have less errors.

Calling log2 internally on log(?, 2) is good for me. Should I create a new PR or can you reopen this one?

Also for consistency I think the already implemented methods log1p and log10 should than be marked as deprecated and it's very simple to be done on log

@dsp
Copy link
Member

dsp commented Sep 16, 2013

Please create a new PR. Also ensure that we check if log2 is available on the system and ifdef the code if necessary. PHP relys on C89 and not C99 and has to work with multiple compilers like ICC/SunCC, etc on different platforms (including Solaris, AIX, etc). Not all of them might have a C99 compatible libc.

@nikic
Copy link
Member

nikic commented Sep 16, 2013

@marc-mabe log1p can't be implemented using log because it has different accuracy requirements. Same for expm1 and exp.

@marc-mabe
Copy link
Contributor Author

@nikic you are right - but log[2|10](x) is the same as log(x, [2|10]) with this patch (https://github.com/marc-mabe/php-src/blob/feature/log_improvement/ext/standard/math.c#L697)

So log10 can be deprecated. Does this need an RFC?

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.

4 participants