{"action":"created","issue":{"url":"https://api.github.com/repos/fabsx00/joern/issues/80","labels_url":"https://api.github.com/repos/fabsx00/joern/issues/80/labels{/name}","comments_url":"https://api.github.com/repos/fabsx00/joern/issues/80/comments","events_url":"https://api.github.com/repos/fabsx00/joern/issues/80/events","html_url":"https://github.com/fabsx00/joern/issues/80","id":117402507,"number":80,"title":"Some refactorings to abstract from C and PHP","user":{"login":"einheitlix","id":1785865,"avatar_url":"https://avatars.githubusercontent.com/u/1785865?v=3","gravatar_id":"","url":"https://api.github.com/users/einheitlix","html_url":"https://github.com/einheitlix","followers_url":"https://api.github.com/users/einheitlix/followers","following_url":"https://api.github.com/users/einheitlix/following{/other_user}","gists_url":"https://api.github.com/users/einheitlix/gists{/gist_id}","starred_url":"https://api.github.com/users/einheitlix/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/einheitlix/subscriptions","organizations_url":"https://api.github.com/users/einheitlix/orgs","repos_url":"https://api.github.com/users/einheitlix/repos","events_url":"https://api.github.com/users/einheitlix/events{/privacy}","received_events_url":"https://api.github.com/users/einheitlix/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":11,"created_at":"2015-11-17T17:12:01Z","updated_at":"2015-11-23T17:45:41Z","closed_at":null,"body":"As discussed in PR #79, I would suggest refactoring some classes within the `ast` package in order to abstract from C and from PHP. Basically every class in `ast` itself should be seen as a base class that can be extended by C- or PHP-aware implementations in `ast.c` resp. `ast.php` packages (the same goes, of course, for packages such as `ast.expressions`, `ast.declarations`, etc.).\r\n\r\nMy basic approach would be to say, \"whenever a base class declares a field that is only used for C (resp. only for PHP), create a new extending class, declare that field in the extending class only, and move associated get- resp. set- methods to the extending class accordingly.\"\r\n\r\nAnother problem: Often, the C implementations override the methods `addChild(ASTNode)` and `accept(ASTNodeVisitor)`, neither of which is needed by the PHP classes. I am not sure what to do about these. The absolutely 100% clean way would be to also move them to extending classes, but this would imply having to create C-aware extending implementations for each and every single class in the `ast` package. Therefore, as long as they do not hurt the PHP world, we can perhaps simply leave them in the base classes, and only move them to extending C-aware classes when such an extending class is declared anyway, due to the approach mentioned above.\r\n\r\nNote that `accept(ASTNodeVisitor)` never hurts PHP world. At the contrary, `addChild(ASTNode)` can be confusing for PHP world, because in PHP world we *do* need the super-method `ASTNode.addChild(ASTNode)` in order to add a node to another node's list of children, but we do *not* need the `addChild(ASTNode)` override declared in most classes that inherit from `ASTNode` and which call various handlers which are not needed for PHP and can have undesired side effects. The override, however, prevents us from calling the super-method `ASTNode.addChild(ASTNode)` directly in PHP world. Currently this is solved by having the edge interpreter in PHP call the various handler methods directly, and have the handler methods call the super-method `ASTNode.addChild(ASTNode)`.\r\n\r\nAll in all, attending to this issue will split the C and the PHP world cleanly and more easily allow for integration of further languages into Joern. I will add one comment to this thread per class that I think needs refactoring. Once that refactoring is done, the corresponding comment can be deleted, or marked in some way. :blush: \r\n\r\n"},"comment":{"url":"https://api.github.com/repos/fabsx00/joern/issues/comments/159008773","html_url":"https://github.com/fabsx00/joern/issues/80#issuecomment-159008773","issue_url":"https://api.github.com/repos/fabsx00/joern/issues/80","id":159008773,"user":{"login":"einheitlix","id":1785865,"avatar_url":"https://avatars.githubusercontent.com/u/1785865?v=3","gravatar_id":"","url":"https://api.github.com/users/einheitlix","html_url":"https://github.com/einheitlix","followers_url":"https://api.github.com/users/einheitlix/followers","following_url":"https://api.github.com/users/einheitlix/following{/other_user}","gists_url":"https://api.github.com/users/einheitlix/gists{/gist_id}","starred_url":"https://api.github.com/users/einheitlix/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/einheitlix/subscriptions","organizations_url":"https://api.github.com/users/einheitlix/orgs","repos_url":"https://api.github.com/users/einheitlix/repos","events_url":"https://api.github.com/users/einheitlix/events{/privacy}","received_events_url":"https://api.github.com/users/einheitlix/received_events","type":"User","site_admin":false},"created_at":"2015-11-23T17:45:41Z","updated_at":"2015-11-23T17:45:41Z","body":"`ast.expressions.ConditionalExpression`\r\n\r\n* For C world:\r\n    * Overide method `addChild(ASTNode)` and have it call `setCondition(ASTNode)`, `setTrueExpression(ASTNode)` or `setFalseExpression(ASTNode)` according to what node is currently being added.\r\n    * Maybe refactor C world to call `getCondition()`, `getTrueExpression()` or `getFalseExpression()` on conditional expressions rather than `getChild(0)`, `getChild(1)` or `getChild(2)`, respectively.\r\n\r\nAlso see PR #82.\r\n"}}