Three Classifiers #7

Normand-1024 · 2017-03-17T15:36:26Z

Comma, assignment-as-values and omitted-curly-braces classifiers with their related tests

Shared functions: paren-node? value-consuming-children and remove-wrapper moved from post-increment to classifier-util.clj

Assignment-as-values and omitted-curly-braces tests use sets instead of vectors to compare line numbers due to the new filter-tree function. It hinders the ability to locate multiple atoms on the same line.

dgopstein · 2017-03-17T16:57:43Z

This commit is great! There are only 3 minor things you should fix though, before we merge:

I added a failing test for curly braces. You test explicitly for ExpressionStatements and NullStatements, but maybe it makes more sense to test for Not CompoundStatement??
Please switch the tests over from listing line numbers to using the <true>/<false> convention. If you need an example take a look at the tests for post-*crement-atom?
Please rename atoms: curly-brace-atom? makes it sound like curly braces are bad, but instead omitting the curly braces is what's bad.

P.S.
I fixed filter tree so it's sequential again, but I still think listing a whole bunch of line numbers in a test is hard to read...

dgopstein · 2017-03-17T15:47:14Z

src/test/resources/assignment-as-value.c

+
+    printf("%d %d %d\n", V1, V2, V3);
+}
+


Can you please use the <true>/<false> convention here, so that tests and their expected outcomes are in the same place? As it is, I can't see what's supposed to happen for each of these liens. Also, it lets you change the test file without having to update a list of 20 indexes that all get offset when add a new line.

dgopstein · 2017-03-17T15:51:44Z

test/atom_finder/classifier_test.clj

+                      (filter-tree assignment-as-value-atom?)
+                      (map loc)
+                      (map :line)
+                      ((partial into #{})))]


((paritial into #{})) -> (into #{})

dgopstein · 2017-03-17T15:52:32Z

src/atom_finder/classifier/curly-braces.clj

+  (let [omittable-types [IASTForStatement IASTIfStatement IASTWhileStatement IASTDoStatement  ICPPASTRangeBasedForStatement]]
+  (cond
+   (some #(instance? % node) omittable-types)
+      (some #(omitted-curly-brace? %) (children node))


#(omitted-curly-brace? %) -> omitted-curly-brace?

dgopstein · 2017-03-17T16:03:38Z

src/atom_finder/classifier.clj

@@ -49,6 +49,9 @@
   (ValidatedAtom :reversed-subscript        reversed-subscript-atom?    (default-finder reversed-subscript-atom?))
   (ValidatedAtom :literal-encoding          literal-encoding-atom?      (default-finder literal-encoding-atom?))
   (ValidatedAtom :post-increment            post-*crement-atom?         (default-finder post-*crement-atom?))
+   (ValidatedAtom :comma                     comma-atom?                      (default-finder comma-atom?))
+   (ValidatedAtom :curly-braces              curly-braces-atom?               (default-finder curly-braces-atom?))


can you rename the functions to match the atom names? comma -> comma-operator and curly-braces -> omitted-curly-braces

dgopstein · 2017-03-17T16:46:25Z

src/atom_finder/classifier/curly-braces.clj

+      (some #(omitted-curly-brace? %) (children node))
+
+   (instance? IASTSwitchStatement node) (not(->> node
+                                              ((partial get-in-tree [1]))


In general, you should never have to use (partial) inside of a (->>), since they kinda do the same thing.
((partial get-in-tree [1])) -> (get-in-tree [1])

Normand-1024 · 2017-03-18T01:54:57Z

-Adjusted some codes based on your comments

-I adjusted the curly braces logic based on your fail tests
-For your fail test at line 106, the else actually follows the second if statement at line 105, which is interesting

-I added a function in util.clj called print-node, sample code:
(->> "omitted-curly-braces.c" resource-path tu (filter-tree omitted-curly-braces-atom?) (map print-node))
Hopefully you find the idea helpful. There's definitely room to improve to make the function more versatile and useful

dgopstein · 2017-03-18T01:50:06Z

src/atom_finder/util.clj

@@ -404,3 +403,18 @@
  (binding [*out* *err*] (println s)))

 (defn all-preprocessor [node] (.getAllPreprocessorStatements (root-ancestor node)))
+
+(defn print-node


Awesome! This is great :)

dgopstein · 2017-03-18T04:08:30Z

test/atom_finder/classifier_test.clj

+                      (map loc)
+                      (map :line))]
+
+      (is (= lines [8 10 12 12 14 16 20 22 30 32 36 38 42 48 50 52 52 56 58 60 62])))))


Before when I mentioned the <true>/<false> test thing, I was talking about the test-atom-lines macro. You call it like this:

(test-atom-lines "type-conversion.c" "<true>" (default-finder type-conversion-atom?)))

What it does is it parses the file in the first argument (in this case type-conversion.c), and it finds every line that has a pattern in it (in this case <true>), and it makes sure that the atom returns true for each of those lines. It's the same as doing (is (= lines [8 10 12 ...])) except that the list of lines is built automatically based on which lines in your C file have <true> written on them.

Please swap out this test to use test-atom-lines. You can see a full example in type_conversion_test.clj.

For some reason cider says unable to resolve symbol when I try to user test-atom-lines in repl.

Also do you just type //<true> <true> if two atoms are expected in the same line

Hmm, not sure about the two atoms on same line, you might actually have to update the macro to get that to work since none of my previous atoms had that behavior.

Sometimes with cider you need to go to the file where the function/macro is defined in (I think it's test_util.clj) and eval (run) the definition. If that doesn't work, also try eval'ing the (ns) call at the top of the file in classifiers.clj and at the top of the file you're working in.

dgopstein · 2017-03-18T04:10:51Z

P.S. I just resolved some conflicts between your branch and the one I just pushed, please make sure to pull before going and making any changes!

dgopstein · 2017-03-18T22:02:52Z

Your code looks great, as far as I'm concerned it's ready to ship. Do you know why the build is failing though??

Normand-1024 · 2017-03-18T22:05:07Z

Works on my end. So I'm not really sure, if it works on your machines, then I don't think it'll be a problem?

dgopstein · 2017-03-18T22:07:19Z

I don't know, I haven't pulled your branch yet (working on something else). We can't leave the build server broken or else its useless. If it's working fine on your end it might be an issue with the travis.cfg? I'll take a look...

dgopstein · 2017-03-18T22:42:55Z

Your tests were in fact broken. I fixed them for you, but next please make sure to run lein test when the build breaks. Travis-CI is there to let you know you have a problem.

Normand-1024 · 2017-03-18T22:48:37Z

I did run lein test before every commit and all the tests were find on my machine. Can you explain what was the problem?

dgopstein · 2017-03-18T22:51:37Z

You can see the fixes here: a42a227

Basically both the .c files you specified had spaces at the end of their strings. See for example: a42a227#diff-f4f175e02cf25ead54beaa240dc4bc16L179

Normand-1024 and others added 7 commits February 20, 2017 12:32

Added configuration file

72568bd

update big-root

f78361d

update with original

71080da

update with master

1d8b7fc

Three Classifiers

d840fdc

three classifiers

9ded5d7

changing tests from vector to sets

0e07ac7

dgopstein reviewed Mar 17, 2017

View reviewed changes

- adjust curly logic and tests -added print-node function

6291ab7

dgopstein reviewed Mar 18, 2017

View reviewed changes

Merge branch 'master' into master

633b2dd

Changed assignment and curly braces test with test-atom-lines

4909b7a

Fix Henry's tests

a42a227

dgopstein merged commit 79a169a into dgopstein:master Mar 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Three Classifiers #7

Three Classifiers #7

Normand-1024 commented Mar 17, 2017 •

edited

Loading

dgopstein commented Mar 17, 2017

dgopstein Mar 17, 2017

dgopstein Mar 17, 2017

dgopstein Mar 17, 2017

dgopstein Mar 17, 2017

dgopstein Mar 17, 2017

Normand-1024 commented Mar 18, 2017 •

edited

Loading

dgopstein Mar 18, 2017

dgopstein Mar 18, 2017

Normand-1024 Mar 18, 2017

Normand-1024 Mar 18, 2017 •

edited

Loading

dgopstein Mar 18, 2017

dgopstein commented Mar 18, 2017

dgopstein commented Mar 18, 2017

Normand-1024 commented Mar 18, 2017

dgopstein commented Mar 18, 2017

dgopstein commented Mar 18, 2017

Normand-1024 commented Mar 18, 2017

dgopstein commented Mar 18, 2017

Three Classifiers #7

Three Classifiers #7

Conversation

Normand-1024 commented Mar 17, 2017 • edited Loading

dgopstein commented Mar 17, 2017

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 commented Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Normand-1024 Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgopstein commented Mar 18, 2017

dgopstein commented Mar 18, 2017

Normand-1024 commented Mar 18, 2017

dgopstein commented Mar 18, 2017

dgopstein commented Mar 18, 2017

Normand-1024 commented Mar 18, 2017

dgopstein commented Mar 18, 2017

Normand-1024 commented Mar 17, 2017 •

edited

Loading

Normand-1024 commented Mar 18, 2017 •

edited

Loading

Normand-1024 Mar 18, 2017 •

edited

Loading