r47200 MediaWiki - Code Review archive

+ function haveBC() { ... + if ($haveBC) // Set to precision of 14. + bcscale(16);

A function called "haveBC" should *not* have side-effects. This should be initialized separately.

(Why do you even need to cache this? Is extension_loaded slow?)

Also, why don't you make this configurable?

+ if ($haveBC) + $stack[] = bcmul( '-1', $arg ); + else + $stack[] = -$arg; [* about 5 million]

All this stuff is ugly. Why don't you just use compatibility functions like we usually do?

	if ( !extension_loaded( 'bcmath' ) ) {
		function bcmul( $left, $right ) { return $left * $right; }
		...
	}

etc.

+ // PHP seems to treat "0" and "" appropriately for this to work.

				$stack[] = ( $left && $right ) ? 1 : 0;

That comment sounds evil. Are you sure you can't have stuff like "0.0" gumming up the gearworks here?

+ if ($haveBC) // Inaccurate, I know... + $stack[] = bcpow( exp(1), $arg );

Why not just write in the value of e as a constant string here to a bunch of digits?

+ if ($haveBC) // ln(x) = 1^(1/e)

Smoking some of the crack, Werdna?  :) A constant can't equal ln(x). I guess you mean ln(x) = x^(1/e). But this is wrong; I don't know where you got it. ln(x) is logarithmic but x^(1/e) is polynomial. More specifically, ln(1) = 0 but 1^(1/e) = 1. ln(e) = 1 but e^(1/e) > 1. Etc.

I don't think there's any way to write log exactly in terms of the operations bcmath makes available. It's likely possible to do some kind of approximation, but I suspect only very slowly.

+function bcround($strval, $precision = 0) { ... +function bcabs( $strval, $precision = 0 ) { ... +function bcceil($strval, $precision = 0) {

These are confusing names to use. They sound like they're part of the extension.

--- trunk/extensions/ParserFunctions/testExpr.php (revision 0) +++ trunk/extensions/ParserFunctions/testExpr.php (revision 47200)

Why don't you use ordinary parser tests so that at least some people will be checking them on a regular basis?

Some fixes in r47203. I was confusing ln() with root base e.

Caching isn't strictly necessary, but it saves some time. Since it's checked per-operation, an average page might check it 1000-10000 times (or somewhere around that order of magnitude). Some quick checking indicates that this would be on the order of 15-150ms, not a huge time difference, but non-trivial.

You're right about '0.0', and a quick test indicates that it might give that output. I'm thinking about how to best work around that.

Writing in the value of e as a constant string would be even more inaccurate than exp(1)

"Caching isn't strictly necessary, but it saves some time. Since it's checked per-operation, an average page might check it 1000-10000 times (or somewhere around that order of magnitude). Some quick checking indicates that this would be on the order of 15-150ms, not a huge time difference, but non-trivial. "

This would be avoided if you used compat functions (which I think is better style anyway).

"Writing in the value of e as a constant string would be even more inaccurate than exp(1) "

Why?

-					$stack[] = bcpow( exp(1), $arg );
+					$stack[] = bcpow( '2.71828182845904523536', $arg );

andrew@voltaire:~$ php -r 'print bcsub(exp(1), 2.71828182845904523536, 50 );'
0.00000000000000000000000000000000000000000000000000

Seems to make no difference, so why go with the less readable version?

You left off the string delimiters, so of course it makes no difference.

0 20:07:20 /var/www/git-trunk$ php -r 'print bcsub(exp(1), "2.71828182845904523536", 20 ) . "\n";'
0.00000000000095476464