Implicit Predicate Atom #12
Conversation
There was a problem hiding this comment.
This is great Jake! There are couple features missing, but this code is 90% of the way there.
These comments start with the important ones, and get less significant as the list goes on:
- Currently your code classifies
if (!(x == y));as an atom, but I don't think it ought to. - I don't think you included a complete list of predicate contexts, i.e. places where predicates are used. Can you think of a way find an exhaustive list?
- Every time we make a new atom classifier we need to add it to the list in classifiers.clj before it'll get used in analyses. Would you mind putting this in there?
| (in-ns 'atom-finder.classifier) | ||
| (import '(org.eclipse.cdt.core.dom.ast IASTIfStatement IASTForStatement IASTWhileStatement IASTDoStatement IASTBinaryExpression)) | ||
|
|
||
| (def logical-operators #{IASTBinaryExpression/op_equals |
There was a problem hiding this comment.
This list looks like it might be useful in other contexts as well. Could you put this in classifier_util.clj??
| IASTBinaryExpression/op_logicalOr | ||
| IASTBinaryExpression/op_notequals}) | ||
|
|
||
| (defn implicit-preciate-for-if-statement? |
| (cond | ||
| (instance? IASTBinaryExpression (.getCondition node)) | ||
| (not (contains? logical-operators (.getOperator (.getCondition node)))) | ||
| :else true)) |
There was a problem hiding this comment.
implicit-predicate-while-do-loop? and implicit-preciate-for-if-statement are basically identical, and they both throw exceptions if you pass in the wrong type. I think you can fix both issues by moving the .getConditionExpression/.getCondition outside the function into the calling code
| (instance? IASTIfStatement node) (implicit-preciate-for-if-statement? node) | ||
| (instance? IASTWhileStatement node) (implicit-predicate-while-do-loop? node) | ||
| (instance? IASTDoStatement node) (implicit-predicate-while-do-loop? node) | ||
| :else false)) |
There was a problem hiding this comment.
If you wanted you could refactor this to use some or atom-finder.util/any-pred instead of cond, and it would reduce a little bit of repetition.
|
Hey Dan, |
There was a problem hiding this comment.
The key to functional programming is composition. Writing general functions that you can reuse in multiple places, then combining them together. This goes hand-in-hand with the “single-responsibility-principle” which is to say that every function should do just one thing. If each function only does one, reasonably general thing, they can easily be reused and mixed together (composed) to make useful things. Another useful one is “don’t repeat yourself”, so like, any time you see repeated patterns in code (maybe anything that’s copy-pasted more than twice), it should be refactored out into one common function or something.
I still think you’re missing some contexts. When I looked through the spec I saw they were using the phrase “controlling expression” to describe the predicates in if statements, from there I could see they always used the phrase “unequal to 0” or “equal to 0” to describe the comparison. By searching those phrases I was able to come up with the following contexts:
- Unary !
- &&/||
- ?:
- If
- While
- Do
- For
- assert
However, Sam pointed out that C++ also has extra contexts that C doesn’t have. He mentioned cast to Bool. I haven’t checked the C++ spec, but could you give that a search and see what other contexts we might be missing?
Also, it looks like you have some merge conflicts. Have you looked into those at all?
| @@ -43,7 +43,7 @@ | |||
|
|
|||
| (def atoms | |||
| [ | |||
| (ValidatedAtom :preprocessor-in-statement define-parent? non-toplevel-defines) | |||
| (ValidatedAtom :preprocessor-in-statement preprocessor-parent? all-non-toplevel-preprocessors) | |||
| (== IASTUnaryExpression/op_not (.getOperator expr)) true | ||
| (== IASTUnaryExpression/op_bracketedPrimary (.getOperator expr)) | ||
| (not (implicit-expression? (.getOperand expr))) | ||
| :else false)) |
There was a problem hiding this comment.
It doesn't look like this function removes parentheses, it looks like applies heuristics to unary operators. If instead you made it actually remove parentheses then you wouldn't need the cyclic reference either, you could just do something like:
(->> node remove-parentheses implicit-expression?)
Also, fix your indentation.
|
|
||
| (deftest test-implicit-predicate-atom? | ||
| (testing "implicit-predicate-atom? finds all atoms in sample code") | ||
| (test-atom-lines "implicit-predicate.c" "<true>" (default-finder implicit-predicate-atom?))) |
There was a problem hiding this comment.
Can you add individual tests to test at the node (not line) level? See for example: https://github.com/dgopstein/atom-finder/blob/master/test/atom_finder/classifier_test.clj#L14
Then add a few tests. For example I'm curious what your code says for !1 and !1 ? 2 : 3. In theory I think the first one should be an atom, and the second one shouldn't.
I'm not sure if my tests were done properly.
I noticed how when I run lein test it doesn't specify my atom (it also doesn't specify omitted-curly-braces).