Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Add support for hexadecimal, octal and binary values #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for hexadecimal, octal and binary values #40

wants to merge 1 commit into from

Conversation

jubianchi
Copy link
Member

Users can now use three new notations:

  • \x5390x539 for hexadecimal values
  • \b1010100b101010 for binary values
  • \052052 for octal values

We can also mix them in expressions:

- ( \b101010 * ( \x539 / -\052 ) ) / 100
- ( 0b101010 * ( 0x539 / -052 ) ) / 100

The result here will be 13.37

@jubianchi
Copy link
Member Author

Of course, tests are green:

$ vendor/bin/hoa test:run -d Test/ -p 'php\ -n' -D

...

> Total tests duration: 69.98 seconds.
> Total tests memory usage: 6.00 Mb.
> Running duration: 70.29 seconds.

Success (7 test suites, 42/42 test cases, 0 void test case, 0 skipped test case, 33271 assertions)!

The only drawbacks of those notation are they are not compatible with PHP itself so we cannot directly add them to unit tests :/ We could support native syntax (as @Metalaka pointed out in #39 - http://php.net/manual/en/language.types.integer.php) but we would have to forbid constants/ids starting with digits so that we can use 0x... (hex), 0b... (binary) and 0... (octal).

@CircleCode
Copy link
Member

this seems weird, but just thinking out loud: what about allowing constants starting by any digit other than 0 (also applicable to #39)

@jubianchi
Copy link
Member Author

@CircleCode why not :)

what about allowing constants starting by any digit other than 0

Must also apply to ids then ;)

@CircleCode
Copy link
Member

maybe the question is "why did we want to allow constants starting by digits?" → the answer could help to chose one of the possible solutions amongst

@jubianchi
Copy link
Member Author

When talking with @Hywan about supporting constants/ids starting with digits, I talked about "side effects": this one is a side-effects. It prevents us from implementing something interesting with a generic notation.

My opinion is: let's close #39 and change constant names in #38 to something like TWO_OVER_PI instead of 2_OVER_PI and use generic notations here.

@CircleCode
Copy link
Member

I share @jubianchi 's opinion.
This would make us more standard

@Hywan
Copy link
Member

Hywan commented Mar 8, 2016

See my comment in #39.

@Metalaka
Copy link
Member

Metalaka commented Mar 8, 2016

@jubianchi @CircleCode 👍
We love standards :)

@Hywan
Copy link
Member

Hywan commented Mar 9, 2016

👍

@jubianchi jubianchi mentioned this pull request Mar 9, 2016
6 tasks
@@ -65,7 +65,7 @@ public function case_visitor_exhaustively()
new Regex\Visitor\Isotropic(
new LUT\Sampler\Random()
),
9
7
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this value because with 9 the test suite generated more than a million expressions and I could not reach the end of the test... way tooooooo long :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with 8 which produce a reasonable test suite (~200 seconds AFAIR) but ended up getting the #42 bug :/

@jubianchi
Copy link
Member Author

> Total tests duration: 143.03 seconds.
> Total tests memory usage: 6.00 Mb.
> Running duration: 143.20 seconds.

Success (7 test suites, 42/42 test cases, 0 void test case, 0 skipped test case, 103047 assertions)!

return $value;
} elseif ('binary' === $token || 'hex' === $token || 'octal' === $token) {
$out = (float) eval('return ' . $value . ';');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STOP THIS!!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For binary: bindec.
For hexa: hexdec.
For octal: octdec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not work!!!!!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only way to make them work is by trimming litterals before passing them to *dec : bindec(str_replace('0b', '', $binary));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do it. That's by far faster than using an eval. Also, eval is not caught by the opcode cache.

@jubianchi
Copy link
Member Author

thanks for the review. Updates will be done today

@Hywan
Copy link
Member

Hywan commented Mar 11, 2016

❤️

@jubianchi
Copy link
Member Author

@hoaproject/hoackers need review :)

Users can now use three new notations:

* `0x539` for hexadecimal values
* `0b101010` for binary values
* `052` for octal values

We can also mix them in expressions:

```
- ( 0b101010 * ( 0x539 / -052 ) ) / 100
```

The result here will be `13.37`
)
->then
->object($compiler->parse($hexadecimalNumber))
->isInstanceOf('Hoa\Compiler\Llk\TreeNode')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can check the ID and the value of the token (resp. token and hexadecimal).
Same for subsequent test cases.

@Hywan
Copy link
Member

Hywan commented May 9, 2016

Review done. Need a little bit more corrections :-).

@Hywan
Copy link
Member

Hywan commented May 9, 2016

And some test cases for overflows.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants