three classifiers #1

Normand-1024 · 2017-01-16T21:09:24Z

added comma, omitted-curly-braces and decrement-increment classifiers and their simple test cases

dgopstein

Your code is great, you've caught on really quickly. I'm very impressed.
Several of the files have had a ton of cosmetic changes applied to them. Please undo those changes, it makes this diff hard to read and screws up the git history.
You added a bunch of files to src/test/resources/ but haven't added any tests in clojure yet. Could you specifically, you want to write some code that will check whether or not the logic of your functions is accurate. When you run "lein test" leiningen will run the tests and tell you whether or not your code is doing what you thought it should.
I made a lot of comments on the code. Only one or two of them are major (i.e. the fact that decrement-increment isn't an atom, but post-inc/dec and pre-inc/dec both separately are). So don't get intimidated, you did a really good job. If you think I'm wrong about any of my comments (probably a few of them are), please argue your case, I'm just throwing ideas out there.

dgopstein · 2017-01-16T21:38:49Z

src/atom_finder/classifier/curly-braces.clj

+  "Does this AST node an statement that should be followed by curly braces?"
+  [node]
+  (let [statement #{;statement that should be followed by compound statements (curly braces)
+                    IASTIfStatement IASTForStatement IASTWhileStatement}]


This list of "statements" doesn't look exhaustive. Is there a better way to do this than by explicit list?

dgopstein · 2017-01-16T21:39:47Z

src/atom_finder/classifier/curly-braces.clj

+                    IASTIfStatement IASTForStatement IASTWhileStatement}]
+      (cond
+        (leaf? node) false
+        (some #(instance? % node) statement) (not-any? true? (map curly-braces? (children node)))


i think the true? and map are redundant here you should just be able to (not-any? curly-braces? ...

dgopstein · 2017-01-16T21:40:44Z

src/atom_finder/classifier/curly-braces.clj

+        (some #(instance? % node) statement) (not-any? true? (map curly-braces? (children node)))
+        :else false)))
+
+(def curly-braces-atom? curly-braciable-statement?)


This is actually bad practice, I had done it originally before I had the classifier.clj registry. There's no need to name the top-level function anything special any more. Eventually I'll go back and clean up my aliases as well.

dgopstein · 2017-01-16T21:41:52Z

src/atom_finder/classifier/curly-braces.clj

+  (cond
+    (leaf? node) nil
+    (curly-braciable-statement? node) [node]
+    :else (mapcat curly-braces-atoms (children node))))


Why is this implementation better than the default-finder?

For these lines I mostly copied from logic-as-control-flow.clj. Can you explain how default-finder works and the reason you used these lines for logic-as-control-flow?

default-finder is mostly just an alias to atoms-in-tree which you can read here:
https://github.com/dgopstein/atom-finder/blob/master/src/atom_finder/util.clj#L323
basically it just does a post-order traversal (https://en.wikipedia.org/wiki/Tree_traversal#Post-order_2) of the AST applying the classifier to every node.

I'm not sure there's an advantage to the one I wrote for logic-as-control-flow, it might've just been there before I'd made default-finder. If you're curious though, it'd be interesting to benchmark the two and see which is faster!

dgopstein · 2017-01-16T21:43:31Z

src/atom_finder/classifier/decrement-increment.clj

+  [node]
+  (let [u-ops #{ ; all decrement or increment operators
+                IASTUnaryExpression/op_postFixDecr      IASTUnaryExpression/op_postFixIncr
+                IASTUnaryExpression/op_prefixDecr       IASTUnaryExpression/op_prefixIncr}]


Prefix increment and decrement are actually a separate atom. If you want, it'd be great to split this out into two classifiers, but as is, this is actually misclassifying ++x as "post-increment"

dgopstein · 2017-01-16T21:44:40Z

src/atom_finder/classifier/decrement-increment.clj

+  "Does this AST node an statement that contains decrement and increment operators?"
+  [node]
+  (let [statement #{;statement types that might not cause confusion
+                    "ForStatement" "ExpressionStatement"}]


Can you use instance instead of typename? Or argue why this is better?

dgopstein · 2017-01-16T21:45:27Z

src/atom_finder/classifier/decrement-increment.clj

+  (cond
+    (leaf? node) nil
+    (decrement-increment-in-statement? node) [node]
+    :else (mapcat decrement-increment-atoms (children node))))


Why is this better than the default finder?

dgopstein · 2017-01-16T21:45:58Z

src/atom_finder/classifier/logic-as-control-flow.clj

+     (leaf? node) false
+     (instance? IASTBinaryExpression node) (contains? b-ops (.getOperator node))
+     (instance? IASTUnaryExpression node) (contains? u-ops (.getOperator node))
+     :else (= (typename node) "FunctionCallExpression"))))


In general you should avoid committing white-space only changes. It screws up the history.

dgopstein · 2017-01-16T21:46:45Z

src/atom_finder/constants.clj

@@ -9,5 +9,5 @@
 (def ag-repo (gitp/load-repo ag-path))

 (def root (tu (resource-path "logic-as-control-flow.c")))
-(def big-root (tu (expand-home "~/opt/src/github-top-c/php-src/ext/sqlite3/libsqlite/sqlite3.c")))
+;(def big-root (tu (expand-home "~/opt/src/github-top-c/php-src/ext/sqlite3/libsqlite/sqlite3.c")))


Sorry! I shouldn't have left that there! Incidentally this is a very good file to test on, if you get the chance to download php-src, it's pretty valuable.

dgopstein

Here's an example of a test that doesn't use an external file: https://github.com/dgopstein/atom-finder/blob/master/test/atom_finder/classifier_test.clj#L39

Here's an example of a test that does use an external file:
https://github.com/dgopstein/atom-finder/blob/master/test/atom_finder/classifier_test.clj#L65

dgopstein · 2017-01-17T21:42:42Z

usefuls.clj

@@ -0,0 +1,6 @@
+;(write-ast (parse-expr "int c = 1 + 3;"))


Does this file need to be committed?

oops, I was sure I rm'ed it before I commit, don't know why it showed up. So every time I try to push to my repo, the pull request gets updated?

Please remove this file.

dgopstein · 2017-01-18T01:25:03Z

I think after the first pull request, yes, I can stop checking this until you're ready, or you can drop the PR and create a new one when you're done.

…

On Jan 17, 2017 8:14 PM, "Normand-1024" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In usefuls.clj <#1>: > @@ -0,0 +1,6 @@ +;(write-ast (parse-expr "int c = 1 + 3;")) oops, I was sure I rm'ed it before I commit, don't know why it showed up. So every time I try to push to my repo, the pull request gets updated? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOxH0CO9u9w2ZNS3p60_orPMhSzA94Cks5rTWeHgaJpZM4Lk--K> .

… not be exhuastive. Also added a case where if user use in/decrement with conditional operator, the classfier identifies it as not confusing.

dgopstein · 2017-01-25T17:08:58Z

src/atom_finder/classifier.clj

@@ -30,16 +35,21 @@
  [name classifier finder]
  `(Atom. (s/validate AtomName ~name)
          (s/fn ~(symbol (str name "-classifier")) :- Boolean [node# :- IASTNode] (~classifier node#))
-          (s/fn ~(symbol (str name "-finder")) :- [IASTNode] [node# :- IASTNode] (~finder node#))
-  ))
+          (s/fn ~(symbol (str name "-finder")) :- [IASTNode] [node# :- IASTNode] (~finder node#))))


Can you please check if there's a way to turn off auto-formatting of lines you don't edit?

dgopstein · 2017-01-25T17:09:05Z

src/atom_finder/classifier.clj

+   (ValidatedAtom :logic-as-control-flow     logic-as-control-flow-atom?      logic-as-control-flow-atoms)
+   (ValidatedAtom :conditional               conditional-atom?                (default-finder conditional-atom?))
+   (ValidatedAtom :comma                     comma-atom?                      (default-finder comma-atom?))
+   (ValidatedAtom :post-increment-decrement  post-increment-decrement-atom?   post-increment-decrement-atoms)


Why is a custom finder better than the default finder here?

dgopstein · 2017-01-25T17:09:09Z

src/atom_finder/classifier/curly-braces.clj

+  "Does this AST node an statement that should be followed by curly braces?"
+  [node]
+  (let [statement #{;statement that should be followed by compound statements (curly braces)
+                    IASTIfStatement IASTForStatement IASTWhileStatement}]


This is an incomplete list

dgopstein · 2017-01-25T17:09:13Z

src/atom_finder/classifier/post-increment-decrement.clj

+        (not-any? #(instance? % node) statement)
+        (if(some true? (map post-increment-decrement? (children node))) true false)
+
+        :else false)))


The functions in this file seem to be copy-pasted a lot, can you factor out the common logic into shared functions?

dgopstein · 2017-01-25T17:09:25Z

test/atom_finder/classifier_test.clj

+  (testing "small statements" 
+    (is (comma-atom? (parse-expr "1,2")))
+    (is (false? (comma-atom? (parse-expr "int a,b;"))))
+    (is (comma-atom? (parse-expr "num1,num2")))))


These tests are all good, but you can do better. For example, you are trying to commit a file called comma.c, but you never use it in this test... could you?

dgopstein · 2017-01-25T17:09:38Z

test/atom_finder/classifier_test.clj

+         [parse-expr "cout << ++a"     true]
+         [parse-expr "printf(\"All the stuffs\", ++a)"     true]
+         [parse-expr "--a,b"     true]
+         [parse-expr "(true)? ++a: --a" false]


why is this false?

dgopstein · 2017-01-25T17:09:42Z

usefuls.clj

@@ -0,0 +1,6 @@
+;(write-ast (parse-expr "int c = 1 + 3;"))


Please remove this file.

Normand 1024 reversed subscripts

added comma, omitted-curly-braces and decrement-increment classifiers

3556ea0

dgopstein requested changes Jan 16, 2017

View reviewed changes

mostly files copied from the original repo to avoid cosmetic changes

ed5c118

dgopstein reviewed Jan 18, 2017

View reviewed changes

added test cases for comma and in/decrement operators. Test cases may…

2c99194

… not be exhuastive. Also added a case where if user use in/decrement with conditional operator, the classfier identifies it as not confusing.

dgopstein requested changes Jan 25, 2017

View reviewed changes

dgopstein pushed a commit that referenced this pull request Feb 13, 2017

Merge pull request #1 from dgopstein/Normand-1024-reversed_subscripts

53dce80

Normand 1024 reversed subscripts

dgopstein closed this Mar 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

three classifiers #1

three classifiers #1

Normand-1024 commented Jan 16, 2017

dgopstein left a comment

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

Normand-1024 Jan 17, 2017

dgopstein Jan 17, 2017

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

dgopstein Jan 16, 2017

dgopstein left a comment

dgopstein Jan 17, 2017

Normand-1024 Jan 18, 2017

dgopstein Jan 25, 2017

dgopstein commented Jan 18, 2017 via email

dgopstein Jan 25, 2017

dgopstein Jan 25, 2017

dgopstein Jan 25, 2017

dgopstein Jan 25, 2017

dgopstein Jan 25, 2017 •

edited

Loading

dgopstein Jan 25, 2017

dgopstein Jan 25, 2017

three classifiers #1

three classifiers #1

Conversation

Normand-1024 commented Jan 16, 2017

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

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 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

dgopstein commented Jan 18, 2017 via email

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 Jan 25, 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 Jan 25, 2017 •

edited

Loading