-
Notifications
You must be signed in to change notification settings - Fork 87
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
Remove sed for condition #381
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mte90 can you explain better what are you trying to do in some of these changes besides removing the sed dependency?
src/translate/compute.rs
Outdated
let op = match operation { | ||
ArithOp::Add => "+", | ||
ArithOp::Sub => "-", | ||
ArithOp::Mul => "*", | ||
ArithOp::Div => "/", | ||
ArithOp::Div => { | ||
scale = "scale=0;"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that in this way the test works in the majority of cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but we shouldn't sacrifice the precision too. 1/2
should still equal 0.5
and not 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have tests for those cases so we can see if there is an issue or not.
@Mte90 but the |
If you see with the tests everything works in this way.
The echo command is not escaped rightly and the bash script fails. That is part of the other PR as documented. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
d810fea
to
350f621
Compare
So right now there are all the tests, the problem is that there is a wrong escaping of variables in the generated bash so doesn't work. I wrote about it on #381 (comment) |
@Mte90 This PR has many tests failed |
@Ph0enixKM yeah I wrote on #381 (comment) why there are issues that depends on another bug |
@Mte90 Can you provide the Amber code that you used to generate this Bash code? Without it, it'll be hard to decipher what went wrong. |
If you try to compile the I improved the code, now the sed trailing zero there is only for the division in this way the code generated is more clean.
|
let op = match operation { | ||
ArithOp::Add => "+", | ||
ArithOp::Sub => "-", | ||
ArithOp::Mul => "*", | ||
ArithOp::Div => "/", | ||
ArithOp::Div => { | ||
after_bc = sed_regex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a separate variable to store the "regex", you can assign a string literal here, like you do with scale
below. Also, can we please use a more descriptive term than after_bc
, such as strip_zeroes
?
ArithOp::Div => {
strip_zeroes = "| sed \"/\\./ s/\\.\\{0,1\\}0\\{1,\\}$//\"";
"/"
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now is strip zeros, maybe in the futurer is something else.
As there is still the issue with the escaping in the bash generated I am not sure if this PRs is fine as it is now.
src/translate/compute.rs
Outdated
let sed_regex = "/\\./ s/\\.\\{0,1\\}0\\{1,\\}$//"; | ||
let mut scale = ""; | ||
let mut after_bc = ""; | ||
let sed_regex = "| sed \"/\\./ s/\\.\\{0,1\\}0\\{1,\\}$//\""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sed
replacement is suspect in two ways: (i) it makes two separate regex calls, one in the initial test for a single ".", and another in the actual substitution; and (ii) it fails to strip trailing "0"s from "123.000". I would like to propose an alternative, which also IMO has the additional advantage of being slightly more readable:
"| sed \"s/\\(\\.\\(0\\|[0-9]*[1-9]\\)\\)00*$/\\1/\""
This finds a "." followed by either a single "0", or any digits followed by a non "0" digit, and replaces that capture with the capture itself, discarding any subsequent "0"s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suggest ensuring that whatever unit tests we have for this include the following cases (that is, if it's possible to make bc
output the ones with extra "0"s):
0
123
1230
12300
123000
123.0
123.00
123.000
123.45
123.450
123.4500
123.45000
123.0045
123.00450
123.004500
123.0045000
Should output:
0
123
1230
12300
123000
123.0
123.0
123.0
123.45
123.45
123.45
123.45
123.0045
123.0045
123.0045
123.0045
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test right now is that now https://github.com/amber-lang/amber/blob/master/src/tests/validity/div.ab if you can suggest me the calculation to do I will add there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @hdwalters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am crap with math, if you can help me to write the tests to generate numbers like this on a division I can add them.
ArithOp::Modulo => { | ||
math_lib_flag = false; | ||
scale = "scale=0;"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to work:
$ echo 'scale=0;1.4+2.4' | bc
3.8
Did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the unit tests for the modulo works as they are now.
I suggest to check those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too couldn't wrap my head around it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that change is for the modulo stuff, with this change the tests we have right now works.
@@ -29,6 +29,6 @@ pub fun ceil(number: Num): Num { | |||
/// Returns the absolute value of a number | |||
#[allow_absurd_cast] | |||
pub fun abs(number: Num): Num { | |||
if number < 0: return -number | |||
if number < 0: return number * -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this? Why not leave the -number
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because doesn't work with this change, without that change this test fails.
This PR has various issues. The big one that doesn't let me to finish and do all the tests and verify everything is that the bash generated has errors in the escaping as I wrote here #381 (comment) Until that situation is not fixed in Amber itself I can't do all the checks that are need to move on. |
Ref: #366
In this way we are configuring the scale we want to approximate.
Right now some tests doens't work as we have again issues on bash generation:
Instead it should be:
As you can see the eco inside the second eval is not escaped.
I guess the issue is there https://github.com/amber-lang/amber/blob/master/src/modules/shorthand/add.rs#L68
But it is a similar issue of #376 with missing/wrong escaping.