Issue 81910043: Memory Analyzer Void-safety changes
|
Created:
10 years ago by Manus Modified:
10 years ago CC:
developers_eiffel.com Base URL:
https://svn.eiffel.com/eiffelstudio/trunk/Src/library/memory_analyzer/ Visibility:
Public. |
DescriptionMemory Analyzer Void-safety changes
Patch Set 1 #
Total comments: 96
Patch Set 2 : Took comments from first review #
Total comments: 20
Patch Set 3 : Took comments from second review #
Total comments: 2
Patch Set 4 : Final code #
MessagesTotal messages: 20
https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:293: l_result := create {MA_CLASS_STONE}.make (a_item.text) I prefer create {MA_CLASS_STONE} l_result.make (...) https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:295: if attached l_result then I prefer if l_result /= Void then https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:387: if attached l_result as l_r then simpler if l_result /= Void then Result := l_result else create Result end But the initial code is weird anyway, this is typically a code that should never be reached. so. this could be simply check never_reached: False end create Result https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e File ma_memory_state.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode56 ma_memory_state.e:56: check attached_item_founded : false end -- Implied by precondition maybe we could just consider that 0 as result is an acceptable value .. if item_founded is Void (which should not occur due to precondition) so I would suggest require founded_type_set: founded_type /= Void do if attached item_founded as l_item_founded then Result := l_item_founded.count_in_system end (for the precondition, this is just a matter of taste) https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode170 ma_memory_state.e:170: if attached l_result as l_res then do check never_reached: False end create Result end
Sign in to reply to this message.
Thanks for the work. I've put extensive comments so that it helps other when migrating to void-safety. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e File ma_memory_change_mediator.e (left): https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ol... ma_memory_change_mediator.e:283: handle_pick_item (a_item: EV_GRID_LABEL_ITEM): MA_CLASS_STONE I would actually change the signature to `detachable MA_CLASS_STONE'. The underlying vision2 framework supports this. That way we can avoid the precondition (which is not going to be satisfied most of the time by the Vision2 framework). https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ol... ma_memory_change_mediator.e:310: l_grid_data_increased := grid_data_increased Again here `sort_data' only being called from `on_grid_header_click' we can add the attached argument `a_grid_data_increased' to this routine and avoid another if. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ol... ma_memory_change_mediator.e:375: check attached l_result end -- Satisfy void-safe compiler Same as above, check False then end is ok. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:216: check attached_grid_data_increased : false end -- Implied by precondition `set' Looking at the callers of update_grid_increased_content, you will notice that they are 2 callers and that one is checking that `grid_data_increased' is not Void, but not the other one at line 130. I would add a check there and possible add the attached argument `a_grid_data_increased' to this routine. This avoids another if. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:368: create Result.default_create 2 things here. `default_create' won't be available on TUPLE anymore since this is not void-safe. And here the routine is use for typing purposes so it is a case where having `check False then end' is ok. Ideally those routines should look like: xxxxxx_type: A_TYPE -- Some comment here require not_callable: False do check False then end ensure for_typing_only: False end https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e File ma_memory_state.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode56 ma_memory_state.e:56: check attached_item_founded : false end -- Implied by precondition Since you dropped the precondition, and the nature of the function, you can remove the else part. We will return 0 when there is no `item_founded'. https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode171 ma_memory_state.e:171: Result := l_res Same as before, just use `check False then end' https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (left): https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:117: if finding_route_to_once then I would simply use `if l_map /= Void then' instead of `if finding_route_to_once' in the remaining of the routine. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:238: l_grid_data := grid_data As done earlier, I would add the attached argument `a_grid_data' and update callers accordingly. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:566: l_object_table := object_table I would remove the precondition and add the `if attached object_table as l_object_table' at the beginning of the routine instead. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:250: grid_data_not_void: attached grid_data Same as sort_data, I'll add the attached argument `a_grid_data' and update callers. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:474: if attached a_map.item (a_object_id) as l_objects_of_type and attached object_table as l_object_table and attached reference_table as l_reference_table then I would remove the preconditions and keep the if without a check. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:610: if attached l_result as l_res then Use `check False then end' as described elsewhere. https://codereview.appspot.com/81910043/diff/1/ma_references_table.e File ma_references_table.e (right): https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode77 ma_references_table.e:77: if attached l_hash as l_h then Here it is a case where using `has_key' is pointless. The code should be written has: l_hash := relations.item (a_referee) if l_hash = Void then create l_hash.make (20) relations.force (l_hash, a_referee) end l_hash.force ([a_referee, data], a_referrer) https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode96 ma_references_table.e:96: if attached l_hash as l_h then Same as above, use `item' to see if it is attached. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (left): https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ol... ma_route_to_once_searcher.e:186: check l_grid /= Void end -- Implied by precondition `grid_set' Those a key events if the grid is not present they should not do anything. I'll add a global "if attached grid as l_grid then" at the beginning of the routine and I will remove the preconditions as well since Vision2 does not expect them. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ol... ma_route_to_once_searcher.e:353: set: attached reference_table I would change the signature of this routine to take 3 additional attached argument `a_reference_table, a_route_stack and a_visited_references. In addition I would remove the following attributes `reference_table' and `visited_references' since they are only used temporarily. I would do the same for `route_stack' but this requires more update. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ol... ma_route_to_once_searcher.e:440: l_route_stack := route_stack Possibly add `route_stack' as attached argument. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (right): https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:95: if attached l_grid as l_l_grid then I believe that you do not need this check at all. Since the `l_grid' is set on both side the preceeding if statement. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:342: if attached l_reference_table as l_r then I would make `build' take an attached argument `a_reference_table' instead as the caller has already done the check it was attached. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:483: check attached_l_table : false end -- Implied by precondition `set' Remove the precondition and the check as returning False is ok. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_figure_factory.e File object_graph/ma_figure_factory.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_figure_factory... object_graph/ma_figure_factory.e:46: create Result -- If no precondtions or checks are switch on this statement will be executed. However, since it is not implemented according to first check statement it might be that we should remove the feature? Remove the part of removing the feature. It is a deferred routine that we inherit from the graph library and we do not have much choice. Unless we change the graph library to return detachable EG_ITEM instead. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... File object_graph/ma_object_graph_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:86: if attached l_result as l_r then This is a typical case where the original author was too optimistic. The return type should be detachable and callers should be updated to handle that better, probably by creating a special EG_NODE that says `not found'. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:245: require Do we really need the preconditions since the body already checks this. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:307: check attached_l_item : false end -- FIXME: Implied by precondition If we drop the precondition, we can drop this too. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:487: feature I think we can revert that change if we remove the precondition. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e File object_graph/ma_object_node.e (left): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e#... object_graph/ma_object_node.e:63: model: detachable EG_NODE Was this necessary? The ancestor is already expecting `model' to be detachable. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e File object_graph/ma_object_node.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e#... object_graph/ma_object_node.e:178: check unimplemented: False then end Why not keeping the `create' clause and this implementation? https://codereview.appspot.com/81910043/diff/1/object_graph/ma_reference_link.e File object_graph/ma_reference_link.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_reference_link... object_graph/ma_reference_link.e:104: require else Weakening of the precondition is not helping. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_reference_link... object_graph/ma_reference_link.e:182: check unimplemented: False then end Same as with MA_OBJECT_NODE, this need to be properly implemented. https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.e File singletons/ma_icons_singleton.e (right): https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.... singletons/ma_icons_singleton.e:38: attached_pixmap_path : attached internal_pixmap_path This is not helping so I would remove it and instead removed the `not_result_is_empty' postcontion of MA_SHARED_PIXMAP_FACTORY.pixmap_path. https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.... singletons/ma_icons_singleton.e:43: if attached l_result as l_r then -- Just do like before but if Void, create the path using `make_current'. https://codereview.appspot.com/81910043/diff/1/singletons/ma_singleton_factory.e File singletons/ma_singleton_factory.e (right): https://codereview.appspot.com/81910043/diff/1/singletons/ma_singleton_factor... singletons/ma_singleton_factory.e:80: if attached l_item as l_i then Use if attached internal_main_window.item as l_item then https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e File widgets/ma_filter_window.e (right): https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:191: check not_editable_item: False end It is in `add_new_row' that we construct a row where the first item is editable. We should add this in comment. https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:195: check l_last_row_attached : false end `add_new_row' guarantees that a new row was added. Now I wonder if the original author should have used instead `l_last_row := grid.row (grid.row_count)'. https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:304: if attached l_result as l_res then Same as elsewhere use `check False then end'. https://codereview.appspot.com/81910043/diff/1/widgets/ma_grid_check_box_item.e File widgets/ma_grid_check_box_item.e (left): https://codereview.appspot.com/81910043/diff/1/widgets/ma_grid_check_box_item... widgets/ma_grid_check_box_item.e:111: check attached l_parent_2 end -- FIXME: Implied by ...? Actually here the whole routine should check `if attached parent as l_parent' to be sound. `parent' is attached if the item is inserted in the grid. If not inserted then `draw_overlay_pixmap' has nothing to do.
Sign in to reply to this message.
I will look in to the cmments and try to update during next week (latest). When I am ready I send you a new patch.
Sign in to reply to this message.
https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (right): https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:95: if attached l_grid as l_l_grid then On 2014/03/28 18:23:34, Manus wrote: > I believe that you do not need this check at all. Since the `l_grid' is set on > both side the preceeding if statement. But in that case we should replace: if attached grid then l_grid := grid else ... end by: l_grid := grid if l_grid = Void then ... end
Sign in to reply to this message.
https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (right): https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:95: if attached l_grid as l_l_grid then For the code above I completely agree. I was mostly focusing on the added if statement,
Sign in to reply to this message.
Uppdated according to comments https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e File ma_memory_change_mediator.e (left): https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ol... ma_memory_change_mediator.e:283: handle_pick_item (a_item: EV_GRID_LABEL_ITEM): MA_CLASS_STONE On 2014/03/28 18:23:34, Manus wrote: > I would actually change the signature to `detachable MA_CLASS_STONE'. The > underlying vision2 framework supports this. That way we can avoid the > precondition (which is not going to be satisfied most of the time by the Vision2 > framework). Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ol... ma_memory_change_mediator.e:310: l_grid_data_increased := grid_data_increased On 2014/03/28 18:23:34, Manus wrote: > Again here `sort_data' only being called from `on_grid_header_click' we can add > the attached argument `a_grid_data_increased' to this routine and avoid another > if. Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ol... ma_memory_change_mediator.e:375: check attached l_result end -- Satisfy void-safe compiler On 2014/03/28 18:23:34, Manus wrote: > Same as above, check False then end is ok. Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:216: check attached_grid_data_increased : false end -- Implied by precondition `set' On 2014/03/28 18:23:34, Manus wrote: > Looking at the callers of update_grid_increased_content, you will notice that > they are 2 callers and that one is checking that `grid_data_increased' is not > Void, but not the other one at line 130. I would add a check there and possible > add the attached argument `a_grid_data_increased' to this routine. This avoids > another if. Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:293: l_result := create {MA_CLASS_STONE}.make (a_item.text) On 2014/03/28 17:34:07, jfiat_es wrote: > I prefer > > create {MA_CLASS_STONE} l_result.make (...) Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:295: if attached l_result then On 2014/03/28 17:34:07, jfiat_es wrote: > I prefer > > if l_result /= Void then Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:368: create Result.default_create On 2014/03/28 18:23:34, Manus wrote: > 2 things here. `default_create' won't be available on TUPLE anymore since this > is not void-safe. > And here the routine is use for typing purposes so it is a case where having > `check False then end' is ok. > > Ideally those routines should look like: > > xxxxxx_type: A_TYPE > -- Some comment here > require > not_callable: False > do > check False then end > ensure > for_typing_only: False > end Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_change_mediator.e#ne... ma_memory_change_mediator.e:387: if attached l_result as l_r then Changed according to Manus comment On 2014/03/28 17:34:07, jfiat_es wrote: > simpler > > if l_result /= Void then > Result := l_result > else > create Result > end > > > But the initial code is weird anyway, this is typically a code that should never > be reached. > > so. this could be simply > > check never_reached: False end > create Result Done.
Sign in to reply to this message.
Changed according to comments https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e File ma_memory_state.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode56 ma_memory_state.e:56: check attached_item_founded : false end -- Implied by precondition Did like this do if attached item_founded as l_item_founded then Result := l_item_founded.count_in_system end ensure positive_value : Result >= 0 end On 2014/03/28 18:23:34, Manus wrote: > Since you dropped the precondition, and the nature of the function, you can > remove the else part. We will return 0 when there is no `item_founded'. Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode170 ma_memory_state.e:170: if attached l_result as l_res then On 2014/03/28 17:34:07, jfiat_es wrote: > do > check never_reached: False end > create Result > end Done. https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode171 ma_memory_state.e:171: Result := l_res On 2014/03/28 18:23:34, Manus wrote: > Same as before, just use `check False then end' Done.
Sign in to reply to this message.
Manu: I did not understand one of your comments, see comments in file. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (left): https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:117: if finding_route_to_once then I do not understand your suggestion. n 2014/03/28 18:23:34, Manus wrote: > I would simply use `if l_map /= Void then' instead of `if finding_route_to_once' > in the remaining of the routine. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:238: l_grid_data := grid_data On 2014/03/28 18:23:34, Manus wrote: > As done earlier, I would add the attached argument `a_grid_data' and update > callers accordingly. Done. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:566: l_object_table := object_table On 2014/03/28 18:23:34, Manus wrote: > I would remove the precondition and add the `if attached object_table as > l_object_table' at the beginning of the routine instead. Done. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:250: grid_data_not_void: attached grid_data On 2014/03/28 18:23:34, Manus wrote: > Same as sort_data, I'll add the attached argument `a_grid_data' and update > callers. Done. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:474: if attached a_map.item (a_object_id) as l_objects_of_type and attached object_table as l_object_table and attached reference_table as l_reference_table then I removed the checks in the else part. I removed some of the preconditions. I kept these two: a_map_not_void: a_map /= Void has: a_map.has (a_object_id) On 2014/03/28 18:23:34, Manus wrote: > I would remove the preconditions and keep the if without a check. Done. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:610: if attached l_result as l_res then On 2014/03/28 18:23:34, Manus wrote: > Use `check False then end' as described elsewhere. Done.
Sign in to reply to this message.
Manu, please check the resulting code which I added as comments https://codereview.appspot.com/81910043/diff/1/ma_references_table.e File ma_references_table.e (right): https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode77 ma_references_table.e:77: if attached l_hash as l_h then The resulting code became: extend (a_referrer: G; a_referee: H; data: detachable ANY) -- Add new relations require a_referrer_not_void: a_referrer /= Void a_referee_not_void: a_referee /= Void local l_hash: detachable like references_by_referee do l_hash := relations.item (a_referee) if l_hash = Void then create l_hash.make (20) relations.force (l_hash, a_referee) end l_hash.force ([a_referee, data], a_referrer) end On 2014/03/28 18:23:34, Manus wrote: > Here it is a case where using `has_key' is pointless. The code should be written > has: > > l_hash := relations.item (a_referee) > if l_hash = Void then > create l_hash.make (20) > relations.force (l_hash, a_referee) > end > l_hash.force ([a_referee, data], a_referrer) Done. https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode96 ma_references_table.e:96: if attached l_hash as l_h then remove (a_referrer: G; a_referee: H) -- Remove relation between `'a_referrer' and `a_referee' if it exists a relation. require a_referrer_not_void: a_referrer /= Void a_referee_not_void: a_referee /= Void local l_hash: detachable like references_by_referee do l_hash := relations.item (a_referee) if attached l_hash as l_h then l_h.remove (a_referrer) if l_h.is_empty then relations.remove (a_referee) end end end On 2014/03/28 18:23:34, Manus wrote: > Same as above, use `item' to see if it is attached. Done.
Sign in to reply to this message.
https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (left): https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ol... ma_route_to_once_searcher.e:186: check l_grid /= Void end -- Implied by precondition `grid_set' On 2014/03/28 18:23:34, Manus wrote: > Those a key events if the grid is not present they should not do anything. I'll > add a global "if attached grid as l_grid then" at the beginning of the routine > and I will remove the preconditions as well since Vision2 does not expect them. Done. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ol... ma_route_to_once_searcher.e:353: set: attached reference_table I think I have done all suggested changes. On 2014/03/28 18:23:34, Manus wrote: > I would change the signature of this routine to take 3 additional attached > argument `a_reference_table, a_route_stack and a_visited_references. In addition > I would remove the following attributes `reference_table' and > `visited_references' since they are only used temporarily. I would do the same > for `route_stack' but this requires more update. Done. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ol... ma_route_to_once_searcher.e:440: l_route_stack := route_stack On 2014/03/28 18:23:34, Manus wrote: > Possibly add `route_stack' as attached argument. Done. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (right): https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:95: if attached l_grid as l_l_grid then On 2014/03/28 18:23:34, Manus wrote: > I believe that you do not need this check at all. Since the `l_grid' is set on > both side the preceeding if statement. Done. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:342: if attached l_reference_table as l_r then On 2014/03/28 18:23:34, Manus wrote: > I would make `build' take an attached argument `a_reference_table' instead as > the caller has already done the check it was attached. Done. https://codereview.appspot.com/81910043/diff/1/ma_route_to_once_searcher.e#ne... ma_route_to_once_searcher.e:483: check attached_l_table : false end -- Implied by precondition `set' On 2014/03/28 18:23:34, Manus wrote: > Remove the precondition and the check as returning False is ok. Done.
Sign in to reply to this message.
https://codereview.appspot.com/81910043/diff/1/object_graph/ma_figure_factory.e File object_graph/ma_figure_factory.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_figure_factory... object_graph/ma_figure_factory.e:46: create Result -- If no precondtions or checks are switch on this statement will be executed. However, since it is not implemented according to first check statement it might be that we should remove the feature? On 2014/03/28 18:23:34, Manus wrote: > Remove the part of removing the feature. It is a deferred routine that we > inherit from the graph library and we do not have much choice. Unless we change > the graph library to return detachable EG_ITEM instead. Done.
Sign in to reply to this message.
We need to decide if we shall have the preconditions or not. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... File object_graph/ma_object_graph_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:86: if attached l_result as l_r then I added detachable to thre returned staement and cleaned this routine. Then I added a check if attached in the only case I found that used this routine. The calling routine can probably be improved as you suggest but I have not done it. Better to take samll steps. On 2014/03/28 18:23:34, Manus wrote: > This is a typical case where the original author was too optimistic. The return > type should be detachable and callers should be updated to handle that better, > probably by creating a special EG_NODE that says `not found'. Done. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:245: require Well, the routine only does its job when these preconditins are true. On 2014/03/28 18:23:34, Manus wrote: > Do we really need the preconditions since the body already checks this.
Sign in to reply to this message.
Comments and questions. I added Victorian as reviewer since he made some changes that Manu wants to discuss. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e File object_graph/ma_object_node.e (left): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e#... object_graph/ma_object_node.e:63: model: detachable EG_NODE THis was something that Victiran added to make his different tracks work. Might be temporary. On 2014/03/28 18:23:34, Manus wrote: > Was this necessary? The ancestor is already expecting `model' to be detachable. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e File object_graph/ma_object_node.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e#... object_graph/ma_object_node.e:178: check unimplemented: False then end Something that Victorian changed. On 2014/03/28 18:23:34, Manus wrote: > Why not keeping the `create' clause and this implementation? https://codereview.appspot.com/81910043/diff/1/object_graph/ma_reference_link.e File object_graph/ma_reference_link.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_reference_link... object_graph/ma_reference_link.e:104: require else Removed it. On 2014/03/28 18:23:34, Manus wrote: > Weakening of the precondition is not helping. Done. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_reference_link... object_graph/ma_reference_link.e:182: check unimplemented: False then end Also Victorian. I will check with him. We had som discussions about it but I never understood exactly why he wanted to change and why he thought it was ok to do this change. On 2014/03/28 18:23:34, Manus wrote: > Same as with MA_OBJECT_NODE, this need to be properly implemented. https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.e File singletons/ma_icons_singleton.e (right): https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.... singletons/ma_icons_singleton.e:38: attached_pixmap_path : attached internal_pixmap_path I removed the precondition above and the SHARED_PIXMAP_FACTORY postcondition in pixmap_path On 2014/03/28 18:23:34, Manus wrote: > This is not helping so I would remove it and instead removed the > `not_result_is_empty' postcontion of MA_SHARED_PIXMAP_FACTORY.pixmap_path. Done. https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.... singletons/ma_icons_singleton.e:43: if attached l_result as l_r then -- I did like this: pixmap_path: PATH -- Path containing all of the Memory Analyzer icons local l_result: like internal_pixmap_path do l_result := internal_pixmap_path if attached l_result as l_r then -- Result := l_r else create Result.make_current end end On 2014/03/28 18:23:34, Manus wrote: > Just do like before but if Void, create the path using `make_current'. https://codereview.appspot.com/81910043/diff/1/singletons/ma_singleton_factory.e File singletons/ma_singleton_factory.e (right): https://codereview.appspot.com/81910043/diff/1/singletons/ma_singleton_factor... singletons/ma_singleton_factory.e:80: if attached l_item as l_i then On 2014/03/28 18:23:34, Manus wrote: > Use if attached internal_main_window.item as l_item then Done. https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e File widgets/ma_filter_window.e (right): https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:191: check not_editable_item: False end Added the comment. On 2014/03/28 18:23:34, Manus wrote: > It is in `add_new_row' that we construct a row where the first item is editable. > We should add this in comment. Done. https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:195: check l_last_row_attached : false end Yopu mean at row 186 ? On 2014/03/28 18:23:34, Manus wrote: > `add_new_row' guarantees that a new row was added. Now I wonder if the original > author should have used instead `l_last_row := grid.row (grid.row_count)'. https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:304: if attached l_result as l_res then On 2014/03/28 18:23:34, Manus wrote: > Same as elsewhere use `check False then end'. Done. https://codereview.appspot.com/81910043/diff/1/widgets/ma_grid_check_box_item.e File widgets/ma_grid_check_box_item.e (left): https://codereview.appspot.com/81910043/diff/1/widgets/ma_grid_check_box_item... widgets/ma_grid_check_box_item.e:111: check attached l_parent_2 end -- FIXME: Implied by ...? On 2014/03/28 18:23:34, Manus wrote: > > Actually here the whole routine should check `if attached parent as l_parent' to > be sound. `parent' is attached if the item is inserted in the grid. If not > inserted then `draw_overlay_pixmap' has nothing to do. Done.
Sign in to reply to this message.
A few answers here. Once you have integrated those changes, send me another patch and I'll update the review with it. https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e File ma_memory_state.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode56 ma_memory_state.e:56: check attached_item_founded : false end -- Implied by precondition This is good except the postcondition's tag, it should be: ensure item_found_count_non_negative: Result >= 0 The convention being that we should include the name of the routine as part of the tag, and that positive means > 0 whereas non_negative means >= 0. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (left): https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:117: if finding_route_to_once then What I meant is that we have the following property `finding_route_to_once = (l_map /= Void)'. So there is no need to use `finding_route_to_once' but we can use `l_map /= Void' instead and this will help in the void-safety aspects since we won't have to check against its possible Voidness again. https://codereview.appspot.com/81910043/diff/1/ma_references_table.e File ma_references_table.e (right): https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode77 ma_references_table.e:77: if attached l_hash as l_h then Agreed. https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode96 ma_references_table.e:96: if attached l_hash as l_h then Use if `l_hash /= Void' as there is no need for an object test. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... File object_graph/ma_object_graph_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:245: require Indeed but what is wrong in doing nothing if the conditions are not met. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e File object_graph/ma_object_node.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_node.e#... object_graph/ma_object_node.e:178: check unimplemented: False then end I see that now from the graph patch update. We will have to also check against our code using the graph library to see if this makes sense or not. https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.e File singletons/ma_icons_singleton.e (right): https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.... singletons/ma_icons_singleton.e:43: if attached l_result as l_r then -- I would write: if attached internal_pixmap_path as l_result then Result := l_result else create Result.make_current end https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e File widgets/ma_filter_window.e (right): https://codereview.appspot.com/81910043/diff/1/widgets/ma_filter_window.e#new... widgets/ma_filter_window.e:195: check l_last_row_attached : false end Yes but that would require a careful check.
Sign in to reply to this message.
https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e File ma_memory_state.e (right): https://codereview.appspot.com/81910043/diff/1/ma_memory_state.e#newcode56 ma_memory_state.e:56: check attached_item_founded : false end -- Implied by precondition On 2014/04/02 16:54:06, Manus wrote: > This is good except the postcondition's tag, it should be: > > ensure > item_found_count_non_negative: Result >= 0 > > > The convention being that we should include the name of the routine as part of > the tag, and that positive means > 0 whereas non_negative means >= 0. Done. https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (left): https://codereview.appspot.com/81910043/diff/1/ma_object_snapshot_mediator.e#... ma_object_snapshot_mediator.e:117: if finding_route_to_once then On 2014/04/02 16:54:06, Manus wrote: > What I meant is that we have the following property `finding_route_to_once = > (l_map /= Void)'. So there is no need to use `finding_route_to_once' but we can > use `l_map /= Void' instead and this will help in the void-safety aspects since > we won't have to check against its possible Voidness again. Done. https://codereview.appspot.com/81910043/diff/1/ma_references_table.e File ma_references_table.e (right): https://codereview.appspot.com/81910043/diff/1/ma_references_table.e#newcode96 ma_references_table.e:96: if attached l_hash as l_h then On 2014/04/02 16:54:06, Manus wrote: > Use if `l_hash /= Void' as there is no need for an object test. Done. https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... File object_graph/ma_object_graph_mediator.e (right): https://codereview.appspot.com/81910043/diff/1/object_graph/ma_object_graph_m... object_graph/ma_object_graph_mediator.e:245: require I thought it would be easier for the users that use this that we have preconditions that clearly states the preconditions. Otherwise it will just silently accept the call. On 2014/04/02 16:54:06, Manus wrote: > Indeed but what is wrong in doing nothing if the conditions are not met. https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.e File singletons/ma_icons_singleton.e (right): https://codereview.appspot.com/81910043/diff/1/singletons/ma_icons_singleton.... singletons/ma_icons_singleton.e:43: if attached l_result as l_r then -- On 2014/04/02 16:54:06, Manus wrote: > I would write: > > if attached internal_pixmap_path as l_result then > Result := l_result > else > create Result.make_current > end Done.
Sign in to reply to this message.
Some more comments based on the review of just the diffs between the 2 patches. I think we were are getting close. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:130: if attached grid_data_increased as a_grid_data_increased then The return type of `l_state_1.compare (l_state_2)' is attached so there is no need for a test here. Just add an attached local variable: l_increased: attached like grid_data_increased l_increased := l_state_1.compare (l_state_2) grid_data_increased := l_increased update_grid_increased_content (l_increased) https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:273: if attached grid_data_increased as a_grid_data_increased then use `l_' rather than `a_' for object test local. We reserve the `a_' for arguments. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:280: handle_pick_item (a_item: EV_GRID_LABEL_ITEM): detachable MA_CLASS_STONE Signature should actually be `a_item: detachable EV_GRID_ITEM' and there should be no precondition since called by Vision2. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:289: create {MA_CLASS_STONE} l_result.make (a_item.text) Use Result directly. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:291: if l_result /= Void then No need for that, it is ok to return Void. https://codereview.appspot.com/81910043/diff/20001/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (right): https://codereview.appspot.com/81910043/diff/20001/ma_object_snapshot_mediato... ma_object_snapshot_mediator.e:161: update_grid_content(l_grid_data) Cosmetics, always a space before ( https://codereview.appspot.com/81910043/diff/20001/ma_object_snapshot_mediato... ma_object_snapshot_mediator.e:519: end If you keep the precondition `has' you should balance the if `attached a_map.item (a_object_id)' with a `check has_object: False end' to show to the reader of the code that he understands that the author wrote the code in full knowledge that he expected it to be present. https://codereview.appspot.com/81910043/diff/20001/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (right): https://codereview.appspot.com/81910043/diff/20001/ma_route_to_once_searcher.... ma_route_to_once_searcher.e:194: select_all_row (l_grid) One tab too many. https://codereview.appspot.com/81910043/diff/20001/ma_route_to_once_searcher.... ma_route_to_once_searcher.e:323: create visited_references.make (a_reference_table.referee_count) Because it is obvious that `visited_references' is attached, using a `if' is overkill. So use a local variable to create the object, assign this local to `visited_references' and use that local below in the call to `deep_visit_node'. https://codereview.appspot.com/81910043/diff/20001/object_graph/ma_object_gra... File object_graph/ma_object_graph_mediator.e (right): https://codereview.appspot.com/81910043/diff/20001/object_graph/ma_object_gra... object_graph/ma_object_graph_mediator.e:234: figures_exists: world.selected_figures.count > 0 If we keep the preconditions, then we need to remove the test `l_nodes.count > 0' from the code below. And for the attached_item, we have to repeat the object test, and balance it with a check.
Sign in to reply to this message.
All comments are "Done". I send a patch with all the latest changes to Manu. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:130: if attached grid_data_increased as a_grid_data_increased then On 2014/04/02 21:59:27, Manus wrote: > The return type of `l_state_1.compare (l_state_2)' is attached so there is no > need for a test here. Just add an attached local variable: > > l_increased: attached like grid_data_increased > > l_increased := l_state_1.compare (l_state_2) > grid_data_increased := l_increased > update_grid_increased_content (l_increased) Done. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:273: if attached grid_data_increased as a_grid_data_increased then On 2014/04/02 21:59:27, Manus wrote: > use `l_' rather than `a_' for object test local. We reserve the `a_' for > arguments. Done. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:280: handle_pick_item (a_item: EV_GRID_LABEL_ITEM): detachable MA_CLASS_STONE it is a problem to change to EV_GRID_ITEM since it does not have a feature "text" which is used when creating the class MA_CLASS_STONE The other changes I have done. On 2014/04/02 21:59:27, Manus wrote: > Signature should actually be `a_item: detachable EV_GRID_ITEM' and there should > be no precondition since called by Vision2. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:289: create {MA_CLASS_STONE} l_result.make (a_item.text) On 2014/04/02 21:59:27, Manus wrote: > Use Result directly. Done. https://codereview.appspot.com/81910043/diff/20001/ma_memory_change_mediator.... ma_memory_change_mediator.e:291: if l_result /= Void then On 2014/04/02 21:59:27, Manus wrote: > No need for that, it is ok to return Void. Done. https://codereview.appspot.com/81910043/diff/20001/ma_object_snapshot_mediator.e File ma_object_snapshot_mediator.e (right): https://codereview.appspot.com/81910043/diff/20001/ma_object_snapshot_mediato... ma_object_snapshot_mediator.e:161: update_grid_content(l_grid_data) On 2014/04/02 21:59:27, Manus wrote: > Cosmetics, always a space before ( Done. https://codereview.appspot.com/81910043/diff/20001/ma_object_snapshot_mediato... ma_object_snapshot_mediator.e:519: end On 2014/04/02 21:59:27, Manus wrote: > If you keep the precondition `has' you should balance the if `attached > a_map.item (a_object_id)' with a `check has_object: False end' to show to the > reader of the code that he understands that the author wrote the code in full > knowledge that he expected it to be present. Done. https://codereview.appspot.com/81910043/diff/20001/ma_route_to_once_searcher.e File ma_route_to_once_searcher.e (right): https://codereview.appspot.com/81910043/diff/20001/ma_route_to_once_searcher.... ma_route_to_once_searcher.e:194: select_all_row (l_grid) On 2014/04/02 21:59:27, Manus wrote: > One tab too many. Done. https://codereview.appspot.com/81910043/diff/20001/ma_route_to_once_searcher.... ma_route_to_once_searcher.e:323: create visited_references.make (a_reference_table.referee_count) On 2014/04/02 21:59:27, Manus wrote: > Because it is obvious that `visited_references' is attached, using a `if' is > overkill. So use a local variable to create the object, assign this local to > `visited_references' and use that local below in the call to `deep_visit_node'. Done. https://codereview.appspot.com/81910043/diff/20001/object_graph/ma_object_gra... File object_graph/ma_object_graph_mediator.e (right): https://codereview.appspot.com/81910043/diff/20001/object_graph/ma_object_gra... object_graph/ma_object_graph_mediator.e:234: figures_exists: world.selected_figures.count > 0 I have removed the test It did already exists a balanced "check" statement. On 2014/04/02 21:59:27, Manus wrote: > If we keep the preconditions, then we need to remove the test `l_nodes.count > > 0' from the code below. > > And for the attached_item, we have to repeat the object test, and balance it > with a check. Done.
Sign in to reply to this message.
Just a small change. https://codereview.appspot.com/81910043/diff/40001/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/40001/ma_memory_change_mediator.... ma_memory_change_mediator.e:280: handle_pick_item (a_item: detachable EV_GRID_LABEL_ITEM): detachable MA_CLASS_STONE Since Vision2 only guarantees that it will give you a EV_GRID_ITEM, you would risk a catcall here. Thus you should have EV_GRID_ITEM and then do an object test in the code. That is to say: if attached {EV_GRID_LABLE_ITEM} a_item as l_label and then l_label.column_index = 1 then create {MA_CLASS_STONE} Result.make (l_item.text) end
Sign in to reply to this message.
https://codereview.appspot.com/81910043/diff/40001/ma_memory_change_mediator.e File ma_memory_change_mediator.e (right): https://codereview.appspot.com/81910043/diff/40001/ma_memory_change_mediator.... ma_memory_change_mediator.e:280: handle_pick_item (a_item: detachable EV_GRID_LABEL_ITEM): detachable MA_CLASS_STONE On 2014/04/03 19:07:15, Manus wrote: > Since Vision2 only guarantees that it will give you a EV_GRID_ITEM, you would > risk a catcall here. Thus you should have EV_GRID_ITEM and then do an object > test in the code. > > That is to say: > > if attached {EV_GRID_LABLE_ITEM} a_item as l_label and then l_label.column_index > = 1 then > create {MA_CLASS_STONE} Result.make (l_item.text) > end Done.
Sign in to reply to this message.
Sorry for the delay in uploading the patch. I think that we are now good commit but before doing so, I just want to make sure that you have done a little bit of testing. Currently we do not offer any test suite for that library and that's not good. So the only testing we can perform is manual by testing the tool itself.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
