infix-operator-precedence atom #10

Normand-1024 · 2017-04-12T12:53:38Z

also added a missing case in assignment-as-value classifier

…nt-as-value classifier

dgopstein · 2017-04-12T15:00:30Z

I can't figure out how to git right now, but here are some extra tests you should add: 8ac7ed9 there are more that you should add of your own as well, but this is a starting point.
Would you mind adding a test to assignment-as-value for the missing assignment type? Recently I've adopted a policy of adding tests every time I find a bug so that a) I can confirm it was a bug b) I can confirm I've fixed it and c) it doesn't happen again.

It would also be nice to add similar tests to other atoms that include assignment, but if you're feeling lazy I can do that, let me know.

I'm pretty sure I have a similar list of assignments somewhere else. Would you mind seeing if thats true, and if so, putting them both into a function like assignment? in one of the util files?

dgopstein · 2017-04-12T14:10:19Z

src/atom_finder/classifier/assignment-as-value.clj

@@ -7,8 +7,8 @@
  "Is this node an assignment expression"
  [node]
  (and (instance? IASTBinaryExpression node)
-       (contains? #{IASTBinaryExpression/op_assign IASTBinaryExpression/op_binaryAndAssign IASTBinaryExpression/op_binaryXorAssign 
-                    IASTBinaryExpression/op_divideAssign IASTBinaryExpression/op_minusAssign
+       (contains? #{IASTBinaryExpression/op_assign IASTBinaryExpression/op_binaryAndAssign IASTBinaryExpression/op_binaryOrAssign


Excellent catch! Can you add a test for this case in assignment as value?

dgopstein · 2017-04-12T14:15:25Z

src/atom_finder/classifier/infix-operator-precedence.clj

+          IASTBinaryExpression/op_assign IASTBinaryExpression/op_binaryAndAssign IASTBinaryExpression/op_binaryOrAssign
+          IASTBinaryExpression/op_binaryXorAssign IASTBinaryExpression/op_divideAssign IASTBinaryExpression/op_minusAssign
+          IASTBinaryExpression/op_moduloAssign IASTBinaryExpression/op_multiplyAssign IASTBinaryExpression/op_plusAssign 
+          IASTBinaryExpression/op_shiftLeftAssign IASTBinaryExpression/op_shiftRightAssign}]


I'm pretty sure I've written this same list before?

Normand-1024 · 2017-05-20T22:52:02Z

It can be argued that the function returning operator precedence level isn't all that useful because of the fact that the conditional operator and assigment operator are on the same precedence level. I just end up using it to check whether the type or operator of the node is contained in the precedence list.

dgopstein · 2017-05-22T23:52:34Z

I made a lot of other minor remarks, but I think this is the crux of the review. If you can only fix one thing, it should be this:

I agree, as it's implemented/used now get-operator-precedence is not useful.

I think the way you should use precedence-level is (1) add every type of operator to it (so you never have to test it for nil), then (2) remove node-operator-equal? and instead test for #(=by precedence-level node %)

dgopstein · 2017-05-22T22:40:56Z

src/atom_finder/util.clj

@@ -496,6 +496,36 @@

       (println "===================================================")))))

+(defn get-precedence-level


clojure functions don't use the "get" prefix convention, this probably belongs in classifier-util as well.

dgopstein · 2017-05-22T22:42:21Z

src/atom_finder/util.clj

+         IASTBinaryExpression/op_binaryXorAssign 15 IASTBinaryExpression/op_divideAssign 15 IASTBinaryExpression/op_minusAssign 15
+         IASTBinaryExpression/op_moduloAssign 15 IASTBinaryExpression/op_multiplyAssign 15 IASTBinaryExpression/op_plusAssign 15 
+         IASTBinaryExpression/op_shiftLeftAssign 15 IASTBinaryExpression/op_shiftRightAssign 15 CPPASTConditionalExpression 15
+         CPPASTExpressionList 16}]


This list is fine, but if you wanted to get crafty you could use lists and a macro to reduce redundancy dramatically.

dgopstein · 2017-05-22T22:43:37Z

src/atom_finder/util.clj

+         CPPASTExpressionList 16}]
+    (if (instance? IASTBinaryExpression node) (precedence-list (.getOperator node)) (precedence-list (type node)))))
+
+(defn assignment-operator?


Technically only one of the operators involved is the "assignment operator", yet they all perform assignment. I think probably assignment? is the right name. Also, I think this belongs in classifier-util, not util (although, heads up, I'm in the midst of refactoring util right now actually...)

dgopstein · 2017-05-22T22:44:39Z

src/atom_finder/util.clj

+  (let [assignment-list 
+        #{IASTBinaryExpression/op_assign IASTBinaryExpression/op_binaryAndAssign IASTBinaryExpression/op_binaryOrAssign IASTBinaryExpression/op_binaryXorAssign IASTBinaryExpression/op_divideAssign IASTBinaryExpression/op_minusAssign IASTBinaryExpression/op_moduloAssign IASTBinaryExpression/op_multiplyAssign IASTBinaryExpression/op_plusAssign IASTBinaryExpression/op_shiftLeftAssign IASTBinaryExpression/op_shiftRightAssign}]
+
+    (if (instance? IASTBinaryExpression node) (contains? assignment-list (.getOperator node)) false)))


Any time you have a boolean literal in an if you can probably do the same thing better with a logical function (i.e. and or or)

dgopstein · 2017-05-22T22:45:53Z

src/test/resources/infix-operator-precedence.c

+
+  if (0 && 1 || 2 && 3) ; // <true>
+  if ((0 && 1) || (2 && 3));
+  if (0 && (1 || 2) && 3);


can you add to the lines which intentionally shouldn't have an atom. helps to show intentionality instead of forgetfulness.

dgopstein · 2017-05-22T22:52:46Z

src/atom_finder/classifier/infix-operator-precedence.clj

+          IASTBinaryExpression/op_minus IASTBinaryExpression/op_divide IASTBinaryExpression/op_modulo
+          IASTBinaryExpression/op_shiftLeft IASTBinaryExpression/op_shiftRight IASTBinaryExpression/op_greaterThan 
+          IASTBinaryExpression/op_greaterEqual IASTBinaryExpression/op_lessThan IASTBinaryExpression/op_lessEqual CPPASTConditionalExpression}]
+    (if (instance? IASTBinaryExpression node) (contains? non-associative-list (.getOperator node)) (contains? non-associative-list (type node)))))


The lispy way to do this:

(contains? non-associative-list ((if (instance? IASTBinaryExpression node) .getOperator type) node))```

dgopstein · 2017-05-22T22:55:40Z

src/atom_finder/classifier/infix-operator-precedence.clj

+
+(defn node-operator-equal?
+  "compare types two nodes, if both binary expression then compare their operators"
+  [node1 node2]


node-operator-equal? is fine, but I might consider calling it just operator-equal? and making it variadic. Makes it more useful and the code smaller.

dgopstein · 2017-05-22T22:58:00Z

src/atom_finder/classifier/infix-operator-precedence.clj

+     (some #(and (not (nil? (get-precedence-level %)))
+                 (not (node-operator-equal? node %))) (children node)))
+
+   false))


Again, don't put false the return of an if.

dgopstein · 2017-05-22T22:59:42Z

src/atom_finder/classifier/infix-operator-precedence.clj

+     (some #(not (nil? (get-precedence-level %))) (children node))
+     ;;Current node is an associative operator
+     (some #(and (not (nil? (get-precedence-level %)))
+                 (not (node-operator-equal? node %))) (children node)))


you use (not (nil? several times here, but the way clojure handles truthiness means you almost never need (not (nil? unless you care about the difference between nil and false.

dgopstein · 2017-05-22T23:46:19Z

src/atom_finder/util.clj

+  [node]
+  (let [precedence-list
+        {;"currently supports binary operators(not including sizeof), comma operator and conditional operator"
+         IASTBinaryExpression/op_modulo 5 IASTBinaryExpression/op_multiply 5 IASTBinaryExpression/op_divide 5


Why aren't all the precedence levels defined in this function? This should definitely have everything from 1-15 or else it won't really be useful outside your code.

dgopstein · 2017-05-27T14:33:23Z

src/atom_finder/classifier/infix-operator-precedence.clj

+  "Is this node a infix-operator-precedence-atom?"
+  [node]
+  (and
+   (and (precedence-level node)


You shouldn't need to nest ands, just add more operands to the first one

(and (and x y) z) is the same as (and x y z)

dgopstein · 2017-05-27T14:35:44Z

src/atom_finder/util.clj

+  (let [assignment-list 
+        #{IASTBinaryExpression/op_assign IASTBinaryExpression/op_binaryAndAssign IASTBinaryExpression/op_binaryOrAssign IASTBinaryExpression/op_binaryXorAssign IASTBinaryExpression/op_divideAssign IASTBinaryExpression/op_minusAssign IASTBinaryExpression/op_moduloAssign IASTBinaryExpression/op_multiplyAssign IASTBinaryExpression/op_plusAssign IASTBinaryExpression/op_shiftLeftAssign IASTBinaryExpression/op_shiftRightAssign}]
+
+    (and (instance? IASTBinaryExpression node) (contains? assignment-list (.getOperator node)))))


All the functions in util look great, but can you put them in cdt_util instead?

maybe classifier_util is better actually. what do you think?

looking through cdt_util and classifier_util, I get the general sense that classifier_util contains generally higher level functions(if that makes sense to you). So I think it's more fitting to put it in classifier_util

That's a good way to put it. I think you're right about both points.

dgopstein · 2017-05-27T14:38:15Z

src/atom_finder/classifier/infix-operator-precedence.clj

+
+   (if (non-associative? node)
+     ;;Current node is a non-associative operator
+     (some #(precedence-level %) (children node))


Eventually I'm going to add a few more entries to the set of operators in precedence-level. Will that break this code?

What are you actually testing when you're using precedence-level as a predicate? I recommend you make a function that describes what you're testing. It should probably end with a ? and return a boolean.

I'm not sure what you're going to add and it really depends on how eclipse parses and what we consider confusing or not confusing. But I think it'll maintain a good accuracy.

(precedence-level node) is basically testing whether or not this node represents an operator. If it returns a number then yes and testing can go on, if nil then this node can be ignored.

With unary operators taken into account I think the code needs to be reconsidered.

! a && b: we agreed that this code is confusing, and currently my code takes care of it

*p && b: assuming p is pointing to a boolean, I don't think this code is as confusing, but in terms of precedence level it is basically the same as the one above

++a + b: again, basically the same thing, but I don't think it's as confusing as the first one.

It sounds like we'd be better of with a function called operator? or something. But, to be honest, that sounds the same as expression? no?

These are really great questions re !, *, and ++. Could you type them up into an email and send to Justin/the group?

I think operators in this case is a subset of expressions. a as a variable is also parsed as an expression, namely CPPASTIdExpression.

And yes I'll try to type up an email and send it to the group

dgopstein

Overall, I think this PR is great. I made a bunch of comments. Fix the ones you want to, leave the ones you don't. There's only one mandatory comment, which is that you should remove the log-files you committed. But otherwise this is really good. I'll be glad to have it in the codebase!

dgopstein · 2017-07-11T17:49:42Z

src/atom_finder/classifier/operator-precedence.clj

+        children
+        (map operator-group)
+        (remove nil?)
+        (map (fn [child-group] (sort [(operator-group node) child-group])))))


This is a very common pattern of have 2 function arities, both of which do the same thing, one using a predefined value. I usually define the unary version in terms of the binary:

([node] (group-pair node (children node)))

dgopstein · 2017-07-11T17:49:47Z

src/atom_finder/classifier/operator-precedence.clj

+  (let [node-group (operator-group node)]
+    (or (= :arith_unary node-group)
+        (= :not node-group)
+        (= :pointer node-group))))


If you wanted, this function could be a single line

(def rvalue-unary-operator? (comp #{:arith_unary :not :pointer} operator-group))

But in general, I'm more a fan of naming your data than naming your behaviors, so might go:

(def rvalue-unary-ops #{:arith_unary :not :pointer})

dgopstein · 2017-07-11T17:57:23Z

src/atom_finder/classifier/operator-precedence.clj

+(defn specific-confusing-operator-combination?
+  "if the combination of groups of operators exists in this set, then the combination is confusing"
+  [combination]
+  (some #(= % combination) (map sort #{[:de_incr :pointer] [:multiply :add] [:arith_unary :add] [:arith_unary :multiply] [:and :add] [:and :multiply] [:and :arith_unary] [:or :add] [:or :multiply] [:or :arith_unary] [:or :and] [:not :add] [:not :multiply] [:not :arith_unary] [:not :and] [:not :or]


please use shorter lines, this is hard to read

when you map over a set #{}, you get back a seq '(), so I wouldn't even bother using set notation to begin with, because its misleading.

you don't sort the combination, which means that if you pass in [:pointer :de_incr] it won't match even when it should

In total I'd probably do something like:

(def confusing-op-combinations (->> [[:de_incr :pointer] [:multiply :add] ...] (map sort) (into #{}))) (defn specific-confusing-operator-combination? [combination] (confusing-op-combinations (sort combination)))

dgopstein · 2017-07-11T17:59:17Z

src/atom_finder/classifier/operator-precedence.clj

+
+
+;;
+;;The following 3 functions are for the binary operator special case


s/binary/unary/

dgopstein · 2017-07-11T17:59:53Z

src/atom_finder/classifier/operator-precedence.clj

+        (= :not node-group)
+        (= :pointer node-group))))
+
+(defn unbracktted-unary-in-children


What is unbracktted?

dgopstein · 2017-07-11T18:01:04Z

src/atom_finder/classifier/operator-precedence.clj

+(defn unbracktted-unary-in-children
+  "Is this node not a bracket and constains unary operator in its children?"
+  [node]
+  (if (not= "()" (write-node node))


I'd love to see a predicate parens?, but I'd probably implement it (instance? IASTParenthesisExpression) or whatever.

dgopstein

Just confirm that not sorting combination isn't a bug for me...

dgopstein · 2017-07-11T20:41:50Z

src/atom_finder/classifier/operator-precedence.clj

-;;
-(defn rvalue-unary-operator?
+;;====================================
+;;For binary operator special case


do you really mean binary, not unary here?? if so, please explain more, I don't understand

dgopstein · 2017-07-11T20:42:45Z

src/conf/henry.edn

-  :gcc-path "~/opt/src/gcc",
-  :ag-path "~/opt/src/the_silver_searcher",
-  :github-top-c "~/opt/src/github-top-c"
-}


Did you remove this on purpose?

JK, saw you added it back.

dgopstein · 2017-07-11T20:47:17Z

src/atom_finder/classifier/operator-precedence.clj

-  (some #(= % combination) (map sort #{[:de_incr :pointer] [:multiply :add] [:arith_unary :add] [:arith_unary :multiply] [:and :add] [:and :multiply] [:and :arith_unary] [:or :add] [:or :multiply] [:or :arith_unary] [:or :and] [:not :add] [:not :multiply] [:not :arith_unary] [:not :and] [:not :or]
-;;[:compare :and] [:compare :or] // Underclassify
-[:compare :not] [:compare :compare] [:pointer :add] [:cond :arith_unary] [:cond :and] [:cond :or] [:cond :not] [:cond :compare] [:cond :cond] [:non-asso :add] [:non-asso :multiply] [:non-asso :arith_unary] [:non-asso :and] [:non-asso :or] [:non-asso :not] [:non-asso :non-asso] [:field_ref :pointer]})))
+  (some #(= % combination) (map sort [[:de_incr :pointer] [:multiply :add] [:arith_unary :add] [:arith_unary :multiply]


Are you sure it's not wrong to not sort combination I feel like it'll actually get the wrong answer 50% of the time?

Also, this function has to do 60 comparisons or something. But if you put all the operator combinations into a set and store that using def, then use a set lookup, it's only a single comparison. 60x speedup.

(def confusing-combinations (into #{} (map sort [[:de_incr :pointer] [:multiply :add] ...]))) (defn specific-confusing-operator-combination? [combination] (confusing-combinations (sort combination)))

dgopstein · 2017-07-11T21:17:24Z

src/atom_finder/classifier/operator-precedence.clj

-                                      [:cond :cond] [:non-asso :add] [:non-asso :multiply] [:non-asso :arith_unary]
-                                      [:non-asso :and] [:non-asso :or] [:non-asso :not] [:non-asso :non-asso]
-                                      [:field_ref :pointer]])))
+  (some #(= % combination) (into #{} (map sort [[:de_incr :pointer] [:multiply :add] [:arith_unary :add] [:arith_unary :multiply]


thanks for doing this, but it actually misses the main benefit of what I was proposing. the construction of the set (into #{} happens every time specific-confusing-operator-combination? is run, which is lame, additionally, (some #(= % combination) has to loop over every element of the set, where as if you did a set-lookup it's constant-time. So, if you wanted to make this better you could:

put (into #{} (map ... into a def variable outside the function, so it's only created one time ever

don't use (some XXX set) on a set, instead just do (set XXX)

That said, if you're sick of this, we can commit the code now, up to you, just let me know.

dgopstein · 2017-07-11T21:25:37Z

Sorry about all the BS, but I really am very happy with how this turned out. Thanks!

infix-operator-precedence atom, also added a missing case in assignme…

372b991

…nt-as-value classifier

dgopstein requested changes Apr 12, 2017

View reviewed changes

implemented infix operator precedence

1a58abd

Merge branch 'master' into infix-operator-precedence

7054119

dgopstein reviewed May 22, 2017

View reviewed changes

Hongwei (Henry) Zhou and others added 3 commits May 27, 2017 09:04

changed several items based on pull request

3e6ab67

Merge branch 'master' into infix-operator-precedence

ac90964

fixed util.clj

216752e

dgopstein reviewed May 27, 2017

View reviewed changes

Hongwei (Henry) Zhou added 13 commits June 8, 2017 10:24

Added additional tests

e423b42

added update-node

c96aae0

updated with master, but currently has path-finding error

d0c9106

Merge branch 'master' of https://github.com/dgopstein/atom-finder

00be07d

updated with master

6718782

merged from master

1ed1cb2

implemented opt grouping

85bebcc

merged with master

bc33e8b

Merge branch 'master' into infix-operator-precedence

50e9f06

updated infix

848fb6a

Merge remote-tracking branch 'upstream/master'

e2d55e0

Merge branch 'master' into infix-operator-precedence

5ff6b8b

adjusted names and comments

30e404b

dgopstein reviewed Jul 11, 2017

View reviewed changes

Hongwei (Henry) Zhou added 2 commits July 11, 2017 16:30

Made adjustment based on pull request comment

d0fc437

removed log files

09a7cd8

dgopstein reviewed Jul 11, 2017

View reviewed changes

optimized hugely with computer magic

9a210d0

dgopstein reviewed Jul 11, 2017

View reviewed changes

optimized hugely with the correct computer magic

de0a322

dgopstein merged commit 04126c3 into dgopstein:master Jul 11, 2017

Normand-1024 deleted the infix-operator-precedence branch July 11, 2017 21:28

		@@ -496,6 +496,36 @@

		(println "===================================================")))))

		(defn get-precedence-level



		;;
		;;The following 3 functions are for the binary operator special case

infix-operator-precedence atom #10

infix-operator-precedence atom #10

Conversation

Normand-1024 commented Apr 12, 2017

dgopstein commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Normand-1024 commented May 20, 2017

dgopstein commented May 22, 2017 • 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

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

Choose a reason for hiding this comment

Normand-1024 May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgopstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgopstein Jul 11, 2017 • 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

dgopstein left a 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

dgopstein commented Jul 11, 2017

dgopstein commented May 22, 2017 •

edited

Loading

Normand-1024 May 28, 2017 •

edited

Loading

dgopstein Jul 11, 2017 •

edited

Loading