search

hoaproject / Compiler

The Hoa\Compiler library.
https://hoa-project.net/
453 stars 47 forks source link

Improve generateToken behaviour. #27

Closed Metalaka closed 9 years ago

Metalaka commented 10 years ago

Fix #26

Hywan commented 10 years ago

Thanks for the PR. I am asking a peer-review from @CircleCode and @Savageman.

Hywan commented 9 years ago

ping?

Metalaka commented 9 years ago

Rebased, Can be merged when the review is done.

Hywan commented 9 years ago

@Metalaka Could you rebase this PR please? Sorry for the too long delay…

Metalaka commented 9 years ago

Rebase & cs-fixer done.

Hywan commented 9 years ago

I tested with the following example http://hoa-project.net/Literature/Hack/Compiler.html#Uniform_random_generation. Here is one of the possible result:

0 => [�[�false�]�,�true�]�
1 => [�0e-631�,�true�,�-339�]�
2 => [�false�,�false�,�0.545406�]�
3 => [�[�"FH'ga'"�]�]�
4 => [�"�aKq3�"�,�false�]�
5 => [�[�false�]�,�true�]�
6 => [�false�,�[�true�]�]�
7 => [�null�,�null�,�true�]�
8 => [�[�[�null�]�]�]�
9 => [�[�[�false�]�]�]�

Two issues:

  1. We must visit the skip token each time, not only once.
  2. Maybe we can empty/destruct the _skipToken attribute when the generation is over.
  3. What is this skip token :stuck_out_tongue:? Is it a valid value for \s?
Metalaka commented 9 years ago
  1. We must visit the skip token each time, not only once.
  2. Maybe we can empty/destruct the _skipToken attribute when the generation is over.

So instead of caching the result string, we can cache the token and return $this->_tokenSampler->visit($token->getAST());.

  1. What is this skip token :stuck_out_tongue:? Is it a valid value for \s?

Yes it is.

Metalaka commented 9 years ago

@Hywan Your � (à is displayed on Win OS cli) character is the "non-breaking space" character (\x00A0) with a wrong encoding.

See test: http://3v4l.org/MATQg and https://github.com/hoaproject/Regex/pull/21

Hywan commented 9 years ago

Excellent. Just update your commit messages as you did on Hoa\Regex and I will merge them!

Hywan commented 9 years ago

Tests are broken though :negative_squared_cross_mark:.

Hywan commented 9 years ago

Before merging this one, we must merge https://github.com/hoaproject/Json/pull/11.

Metalaka commented 9 years ago

With https://github.com/hoaproject/Json/pull/11, https://github.com/hoaproject/Json/pull/12 and this PR everything seem good. Thanks for PRs on Hoa\Json.

Hywan commented 9 years ago

@Metalaka Going to re-check tomorrow with hoaproject/Json#11 and hoaproject/Json#12 merged. Thanks!

Hywan commented 9 years ago

Thanks for this PR! Awesome job, as usual 😎.

Metalaka commented 9 years ago

Thanks :)