Add relation and equality operators #17

matedabis · 2019-02-08T08:32:23Z

This generator tests ES v5.1 11.8. - 11.9. (except 11.8.7. and 11.8.6.), relation and equality operators (> >= <= < == === != !==).
The new validate function:

function validate_boolean(test_value, expected_result, false_result) {
    assert(test_value === expected_result);
    assert(test_value.toString() === expected_result.toString());
    assert(test_value !== false_result);
    assert(test_value.toString() !== false_result.toString());
}

A test case looks like this:

t1 = false;
t2 = null;
t3 = null;
t4 = false;
t5 = -2933530887918864;
t6 = false;
t7 = NaN;
t8 = '5871211041646662';
t9 = null;
t10 = NaN;
t11 = 5820634256891770;
f = true;
e = false;
validate_boolean(((((((((((t1 != t2) > t3) <= t4) <= t5) !== t6) !== t7) !== t8) >= t9) === t10) > t11)
, e, f);

rerobika · 2019-02-08T08:46:20Z

src/scripts/generator_base.py

-                 declaration_string += "%s = '%s';\n" % (str(key), str(value))
-             else:
-                 declaration_string += "%s = %d;\n" % (str(key), value)
+             declaration_string += "%s = %s;\n" % (str(key), str(value))


This utility function is changes over and over the PRs. This should be finalized somehow.

rerobika · 2019-02-08T08:47:40Z

src/scripts/generate_relation_and_equality_ops.py

+
+    class _InnerValues():
+        generated_filename = '''relation-equality-ops-{NUMBER}.js''' # The file name prefix of the generated
+        relational_and_equality_operators = ["equ", "nequ", "sequ", "snequ", "less", "moreorequ", "more", "lessorequ"]


I'd suggest the: eq, neq, less, less_eq, greater, greater_eq namings.

rerobika · 2019-02-08T08:49:10Z

src/scripts/generate_relation_and_equality_ops.py

+            elif (decider == 3):
+                self.operands[i] = "false"
+            elif (decider == 4):
+                self.operands[i] = "NaN"


How about this?

if decider <= 4: self.operands[i] = special_values[i]

rerobika · 2019-02-08T08:50:32Z

src/scripts/generate_relation_and_equality_ops.py

+            converted = "true"
+        else:
+            converted = "false"
+        return converted


I'd suggest: return "true" if value else "false";

rerobika · 2019-02-08T08:51:22Z

src/scripts/generate_relation_and_equality_ops.py

+            converted = "false"
+        return converted
+
+    # Add comas to the value if it is meant to be a string in the test case


Typo: comas

rerobika · 2019-02-08T08:53:05Z

src/scripts/generate_relation_and_equality_ops.py

+        (left_value == "undefined" and right_value == "null") or
+        (left_value == "null" and right_value == "undefined") or
+        (left_value == "true" and right_value == "true") or
+        (left_value == "false" and right_value == "false")):


Create a mapping or something for these special cases. This look like a but ugly.

matedabis · 2019-02-08T09:59:54Z

Updated the patch.

loki04 · 2019-02-11T07:16:03Z

src/scripts/generate_relation_and_equality_ops.py

+
+    class _InnerValues():
+        generated_filename = '''relation-equality-ops-{NUMBER}.js''' # The file name prefix of the generated
+        relational_and_equality_operators = ["eq", "neq", "seq", "sneq", "less", "less_eq", "greater", "greater_eq"]


The assembly-like form would be much better. Please use the followings or the lowercase version of them instead!
EQ = equal
NE = not equal
GT = greater
LT = lesser
GE = greater and equal
LE = lesser and equal
SE (or STRICT_EQ) = strict equal
SN (or STRICT_NQ) = not strict equal

loki04 · 2019-02-11T07:27:55Z

src/scripts/generate_relation_and_equality_ops.py

+              "seq": self.seq,
+              "neq": self.neq,
+              "sneq": self.sneq,
+        } # Functions for the operators


How about to generate getattr(self, relational_and_equality_operators[i]) for each elements of relational_and_equality_operators ?

loki04 · 2019-02-11T07:29:11Z

src/scripts/generate_relation_and_equality_ops.py

+        } # Functions for the operators
+
+    # Generating the expression with operands and operators
+    def expression_maker(self):


expression_maker? The expression_generator sounds better.

src/scripts/generate_relation_and_equality_ops.py

loki04 · 2019-02-11T08:53:28Z

src/scripts/generate_relation_and_equality_ops.py

+
+        # If it does not match the cases above and values are not undefined or null
+        elif (left_value != "undefined" and left_value != "null" and right_value != "null"
+        and right_value != "undefined"):


elif left_value not in ["undefined", "null"] and right_value not in ["undefined", "null"]: ?

loki04 · 2019-02-11T09:04:47Z

src/scripts/generate_relation_and_equality_ops.py

+            result =  left_converted < right_converted
+        else:
+            result = int(left_converted) < int(right_converted)
+            result = self.convert_true_false_to_lowercase_string(result)


Please combine these expressions!

loki04 · 2019-02-11T09:06:22Z

src/scripts/generate_relation_and_equality_ops.py

+    # Greater than operator
+    def greater(self, left_value, right_value):
+        if (left_value == "NaN" or right_value == "NaN" or
+        left_value == "undefined" or right_value == "undefined"):


?
if left_value in ["NaN", "undefined"] or right_value in ["NaN", "undefined"]:

loki04 · 2019-02-11T09:06:53Z

src/scripts/generate_relation_and_equality_ops.py

+    # Less than or equal operator
+    def less_eq(self, left_value, right_value):
+        if (left_value == "NaN" or right_value == "NaN" or
+        left_value == "undefined" or right_value == "undefined"):


?
if left_value in ["NaN", "undefined"] or right_value in ["NaN", "undefined"]:

loki04 · 2019-02-11T09:07:00Z

src/scripts/generate_relation_and_equality_ops.py

+    # Greater than or equal operator
+    def greater_eq(self, left_value, right_value):
+        if (left_value == "NaN" or right_value == "NaN" or
+        left_value == "undefined" or right_value == "undefined"):


?
if left_value in ["NaN", "undefined"] or right_value in ["NaN", "undefined"]:

loki04 · 2019-02-11T09:10:33Z

src/scripts/generate_relation_and_equality_ops.py

+        write_file(gen_settings, self.file_output)
+        self.debug(Messages.done, self.options)
+
+def main():


Why do you create separate main function? Do you want to call this why you are importing this class?
If you do not want that the main will be callable, please move its body under if __name__ == '__main__': block!

matedabis · 2019-02-12T15:00:17Z

Updated the patch according to the review, and also found and fixed some bugs. I corrected almost everything mentioned, I have to reply to some questions.

expression_maker? The expression_generator sounds better.

I already have a function with that name so its not possible.

?
left_converted = {"true": 1, "false": 0}.get(left_value, int(left_value))

I had to check for true or false before, because it would try to convert the default value to int, and it throws error, this way it is still shorter and simpler than before.

if left_value in ["true", "false"]:
    left_converted = {"true": 1, "false": 0}.get(left_value)
else:
    left_converted = int(left_value)

Why do you create separate main function? Do you want to call this why you are importing this class?
If you do not want that the main will be callable, please move its body under if __name__ == '__main__': block!

I did not want it to make it callable, but this review gave me and idea so I left it as it was, but if you do not agree I will change it. When we reach the end of this project, and we will have more generators, we could create a script which calls all the generators and they each generate the same number of test cases which number is given by argparser, and we could add a command line option to run the generated tests with the test runner script after the generators finished generating, and maybe an another one, which deletes the .js files which did not fail (This would not be useful in all the cases, but as an option). And if this project seems to be useful, we could insert it to jerryscript's run-tests.py with a command line option. What do you think?

loki04 · 2019-02-13T13:59:05Z

Updated the patch according to the review, and also found and fixed some bugs. I corrected almost everything mentioned, I have to reply to some questions.

expression_maker? The expression_generator sounds better.

I already have a function with that name so its not possible.

It sounds weird. Please, try to find a different function name.

?
left_converted = {"true": 1, "false": 0}.get(left_value, int(left_value))

I had to check for true or false before, because it would try to convert the default value to int, and it throws error, this way it is still shorter and simpler than before.

Well, probably I wrote the int() at wrong position. Consider this instead:

for v in [0, 1, 42, -33, "123", "true", "false"]:
  out = int({"true": 1, "false": 0}.get(v, v))
  print(out, type(out))

0 <class 'int'>
1 <class 'int'>
42 <class 'int'>
-33 <class 'int'>
123 <class 'int'>
1 <class 'int'>
0 <class 'int'>

Why do you create separate main function? Do you want to call this why you are importing this class?
If you do not want that the main will be callable, please move its body under if __name__ == '__main__': block!

I did not want it to make it callable, but this review gave me and idea so I left it as it was, but if you do not agree I will change it. When we reach the end of this project, and we will have more generators, we could create a script which calls all the generators and they each generate the same number of test cases which number is given by argparser, and we could add a command line option to run the generated tests with the test runner script after the generators finished generating, and maybe an another one, which deletes the .js files which did not fail (This would not be useful in all the cases, but as an option). And if this project seems to be useful, we could insert it to jerryscript's run-tests.py with a command line option. What do you think?

This can be done and could be a good option, but I still can't see the need of a separate main. In your case we can import classes and use them as you described above.

rerobika · 2019-02-19T06:59:51Z

src/scripts/generate_relation_and_equality_ops.py

+    def eq_helper_special_cases(self, values):
+        special_cases = [["undefined", "undefined"], ["null", "null"], ["undefined", "null"],
+        ["null", "undefined"], ["true", "true"], ["false", "false"]]
+        return True if values in special_cases else False


return values in special_cases is enough

rerobika requested changes Feb 8, 2019

View reviewed changes

matedabis force-pushed the add_binary_features branch 2 times, most recently from 6683c57 to 5ca07d7 Compare February 8, 2019 09:57

loki04 requested changes Feb 11, 2019

View reviewed changes

matedabis force-pushed the add_binary_features branch from 5ca07d7 to ec29d28 Compare February 12, 2019 14:25

matedabis force-pushed the add_binary_features branch from ec29d28 to 7085814 Compare February 14, 2019 11:30

rerobika reviewed Feb 19, 2019

View reviewed changes

Add relation and equality operators

513310b

matedabis force-pushed the add_binary_features branch from 7085814 to 513310b Compare February 19, 2019 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add relation and equality operators #17

Add relation and equality operators #17

matedabis commented Feb 8, 2019 •

edited

Loading

rerobika Feb 8, 2019

rerobika Feb 8, 2019

rerobika Feb 8, 2019

rerobika Feb 8, 2019

rerobika Feb 8, 2019

rerobika Feb 8, 2019

matedabis commented Feb 8, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

loki04 Feb 11, 2019

matedabis commented Feb 12, 2019

loki04 commented Feb 13, 2019

rerobika Feb 19, 2019

Add relation and equality operators #17

Are you sure you want to change the base?

Add relation and equality operators #17

Conversation

matedabis commented Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matedabis commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matedabis commented Feb 12, 2019

loki04 commented Feb 13, 2019

Choose a reason for hiding this comment

matedabis commented Feb 8, 2019 •

edited

Loading