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

Allow constants names to start with digits #39

Closed
wants to merge 1 commit into from
Closed

Allow constants names to start with digits #39

wants to merge 1 commit into from

Conversation

jubianchi
Copy link
Member

$ echo -n '2_OVER_314*2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s

 #  namespace     token name            token value                     offset
-------------------------------------------------------------------------------
 0  default       constant              2_OVER_314                           0
 1  default       times                 *                                   10
 2  default       number                2                                   11
 3  default       EOF                                                       12

@@ -44,12 +44,12 @@
%token bracket_ \(
%token _bracket \)
%token comma ,
%token number (0|[1-9]\d*)(\.\d+)?([eE][\+\-]?\d+)?
%token number (0|[1-9]\d*)(\.\d+)?([eE][\+\-]?\d+)?\b
Copy link
Member

Choose a reason for hiding this comment

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

👍

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 move the number token after the constant token to avoid 7A to be consumed as a number 7 and a constant A instead of the constant 7A.

Copy link
Member

Choose a reason for hiding this comment

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

And this way you might remove \b.

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 was wondering what the best solution was ;) I'll move the lines if you prefer :)

Copy link
Member

Choose a reason for hiding this comment

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

It's better to move the line I guess.

@jubianchi
Copy link
Member Author

@Hywan & @hoaproject/hoackers good for me, let me know if I have to change anything else.

@Metalaka
Copy link
Member

Metalaka commented Mar 7, 2016

Good for me if you have thought about this:

Constants starting with digits can looks like hexadecimal value, so 0X1A is it a constant or the decimal value 26 ?

With this grammar it's a constant since we don't recognize (yet?) hexa/binary values.

http://php.net/manual/en/language.types.integer.php
hexadecimal : 0[xX][0-9a-fA-F]+

@jubianchi
Copy link
Member Author

@Metalaka fair point!

The question is do we plan to support hex. values ? (@Hywan)

If yes, we'll have to find a consistent way to handle them among constants and ids.

@jubianchi
Copy link
Member Author

Here is what I can propose:

  • \x1A for hex values,
  • \0755 for octal values,
  • \b010101 for binary values

I already patched the grammar and the arithmetic visitor:

diff --git c/Arithmetic.pp i/Arithmetic.pp
index 02cc0fe..b948e11 100644
--- c/Arithmetic.pp
+++ i/Arithmetic.pp
@@ -44,13 +44,16 @@
 %token  bracket_  \(
 %token _bracket   \)
 %token  comma     ,
+%token  hex       \\x[0-9a-fA-F]+
+%token  octal     \\0[0-7]+
+%token  binary    \\b[01]+
 %token  constant  [0-9_]*[A-Z_]+[A-Z0-9_]*
+%token  id        \w+
 %token  number    (0|[1-9]\d*)(\.\d+)?([eE][\+\-]?\d+)?
 %token  plus      \+
 %token  minus     \-|−
 %token  times     \*|×
 %token  div       /|÷
-%token  id        \w+

 expression:
     primary() ( ::plus:: #addition expression() )?
@@ -73,7 +76,10 @@ term:
   | function()

 number:
-    <number>
+      <number>
+    | <hex>
+    | <octal>
+    | <binary>

 constant:
     <constant>
diff --git c/Visitor/Arithmetic.php i/Visitor/Arithmetic.php
index a7da754..32d808f 100644
--- c/Visitor/Arithmetic.php
+++ i/Visitor/Arithmetic.php
@@ -255,6 +255,12 @@ class Arithmetic implements Visitor\Visit
                     }
                 } elseif ('id' === $element->getValueToken()) {
                     return $value;
+                } elseif ('binary' === $element->getValueToken()) {
+                    return bindec($value);
+                } elseif ('hex' === $element->getValueToken()) {
+                    return hexdec($value);
+                } elseif ('octal' === $element->getValueToken()) {
+                    return octdec($value);
                 } else {
                     $out = (float) $value;
                 }

I ran this through those tests:

echo -n '2_SQRT_314 * 2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s
echo -n '2xfoo * 2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s
echo -n '0XA * 2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s

echo -n '\x1A * 2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s
echo -n '\0755 * 2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s
echo -n '\b01001 * 2' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -s

echo -n '\b01001' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -c Hoa.Math.Visitor.Arithmetic -s
echo -n '\x1A' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -c Hoa.Math.Visitor.Arithmetic -s
echo -n '\0755' | vendor/bin/hoa compiler:pp Arithmetic.pp 0 -c Hoa.Math.Visitor.Arithmetic -s

P.S: I could easily open a third PR for this feature if we want this ;)

@jubianchi
Copy link
Member Author

Could not resist opening the PR: #40 ;)

@Hywan
Copy link
Member

Hywan commented Mar 8, 2016

Hehe, I am sorry but… this is a Maths library. Did you ever write an hexidecimal value in a Math equation :-p? Personally, no. Thoughts?

@jubianchi
Copy link
Member Author

@Hywan maths are not only decimal. One may need to do binary computations and get a decimal result.

@Savageman
Copy link
Member

I agreee with @jubianchi : Math is just a tool. Mathematicians will use the most appropriate paradigm to solve their problems, and decimal is not always the best tool to solve a problem.

As for syntax, PHP uses natively 0660 for octal, 0x1A for hexa and 0b01001 for binary. Since it's a PHP library it would make sense to use the same?

@Hywan
Copy link
Member

Hywan commented Mar 8, 2016

So a number must be start by 0 and the problem will be solved. It sounds correct too, a lot of language does this.

@jubianchi jubianchi mentioned this pull request Mar 9, 2016
6 tasks
@jubianchi
Copy link
Member Author

let's close this one! (#41 (comment))

@jubianchi jubianchi closed this Mar 9, 2016
@Hywan Hywan removed the in progress label Mar 9, 2016
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