Graph library void-safety issue

[Emmanuel Stapf]

Hi Victorien,

The issue you had was easily solved by changing {EG_FIGURE}.default_create from

default_create

-- Create an EG_FIGURE

do

Precursor {EV_MODEL_MOVE_HANDLE}

create name_label

extend (name_label)

is_center_valid := True

end

to

default_create

-- Create an EG_FIGURE

do

create name_label

Precursor {EV_MODEL_MOVE_HANDLE}

extend (name_label)

is_center_valid := True

end

I'll continue reviewing what you have done at https://github.com/Conaclos/EG-complete-safe

Thanks,

Manu

[Emmanuel Stapf]

We use a code review website, but unfortunately google doesn't let anyone outside the eiffel.com domain use it. Here is the email review that we have done. The links won't unfortunately work for you. If you know a public site where we can put a link to our subversion repository and provide a patch file for review, we can also use this.

Thanks,

Manu

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/graph-safe.ecf#newcode11

graph-safe.ecf:11: <option full_class_checking="true" is_attached_by_default="true" void_safety="all" syntax="standard" namespace="EiffelSoftware.Library.Graph">
full_class_checking, is_attached_by_default, and syntax can be removed.

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/model/eg_item.e#newcode34
model/eg_item.e:34: -- Unique id

Shouldn't period be kept?

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation.e#newcode106

physics/eg_particle_simulation.e:106: check anchor_type_only: False end It's better to change that to

    check False then end

and remove the rest, including local variable.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e#newcode105

physics/eg_particle_simulation_bh.e:105: check l_result /= Void then -- Implied by previsous if cluase and postcondition of `traverse_tree'

I'd better change the previous "if" to "check" and remove this instruction. l_result can be removed as well.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e

File physics/eg_vector2d.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e#newcode112

physics/eg_vector2d.e:112: check l_result /= Void then -- Satisfy void-safe compiler Previous "check false end" can be changed to "check false then end" end the rest, including local variable, can be removed.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e#newcode123

physics/eg_vector2d.e:123: check l_result /= Void then -- Satisfy void-safe compiler Previous "check false end" can be changed to "check false then end" end the rest, including local variable, can be removed.

 

http://patchreview.eiffel.com/5673173882241024/

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/model/eg_linkable.e

File model/eg_linkable.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/model/eg_linkable.e#newcode33

model/eg_linkable.e:33: check attached name as al_name then Actually looking at the code, I don't think we ever verify that `name'

is not Void before using it, so I think we should make this query detachable, or provide a default name if none is set.

 

It is only used by the XML code to generate a valid XML code, but the name comes from the XML to begin with.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_force_directed_layout_n2.e

File physics/eg_force_directed_layout_n2.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_force_directed_layout_n2.e#newcode258

physics/eg_force_directed_layout_n2.e:258: check l_other /= Void then --

FIXME: Implied by...?

It might be more changes but I would recommend removing that and change EG_LINK_FIGURE to setup both `source' and `target' so that they are never Void. In practice, this is what happens since we always create descendants of EG_LINK_FIGURE with a model EG_LINK for which they are never Void.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation.e

File physics/eg_particle_simulation.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation.e#newcode107

physics/eg_particle_simulation.e:107: check l_result /= Void then -- Satisfy void safe compiler Actually here this is for typing only, so the whole body should be replaced by just:

 

    -- Typing only

check False then end

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e

File physics/eg_particle_simulation_bh.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e#oldcode105

physics/eg_particle_simulation_bh.e:105: check l_result /= Void end -- Implied by previsous if cluase and postcondition of `traverse_tree'

I would change the creation procedure of this class to create `quad_tree' and make `quad_tree' attached. That way most of the code doesn't need checks.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e

File physics/eg_particle_simulation_bh.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e#newcode127

physics/eg_particle_simulation_bh.e:127: check attached node.particle as al_particle then -- Implied by `is_leaf'

This is annoying, I would have 2 EG_QUAD_TREEs: EG_QUAD_TREE (where the

4 sub elements are attached and no particle) and EG_QUAD_TREE_LEAF that has a particle which is attached.

 

It might change the way we build the quad tree but seems easier. Now instead of doing node.is_leaf, we just do an object test attached {EG_QUAD_TREE_LEAF} node as l_leaf.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_n2.e

File physics/eg_particle_simulation_n2.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_n2.e#oldcode37

physics/eg_particle_simulation_n2.e:37: Result := l_result I would change the code but since the class has been made obsolete for a long time now and is not in use for us, I would remove the class altogether.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_quad_tree.e

File physics/eg_quad_tree.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_quad_tree.e#oldcode53

physics/eg_quad_tree.e:53: check l_particle /= Void end -- Implied by invariant `leaf_has_particle_inner_nodes_do_not'

See my previous comment about rewritting EG_QUAD_TREE to avoid too much checks by splitting the class in 2 descendants.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_energy.e

File physics/eg_spring_energy.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_energy.e#oldcode102

physics/eg_spring_energy.e:102: check l_result /= Void end -- Satisfy void-safe compiler Use `check False then end' since this is for typing purpose only. Forgot to say it at the previous occurrence, but we should add a precondition `require False' as well, since this is not made to be called.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_particle.e

File physics/eg_spring_particle.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_particle.e#oldcode121

physics/eg_spring_particle.e:121: Result := l_result See other comments about queries used for just typing purpose.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e

File physics/eg_vector2d.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e#newcode43

physics/eg_vector2d.e:43: check attached x as al_x then -- Implied by precondition `set'

We actually need to check callers of those and remove this. It was a bad initial void-safety conversion that is the cause of this code.

 

This is even worse, when you look at the caller such as `zero' there is no precondition to prevent that at all.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_circle_layout.e

File view/eg_circle_layout.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_circle_layout.e#newcode110

view/eg_circle_layout.e:110: check l_cluster /= Void then -- FIXME:

Implied by ...?

This is a command, so I would simply write:

if l_cluster /= Void then

  ... the line of today

else

   check cluster_preset: False end

end

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_cluster_figure.e

File view/eg_cluster_figure.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_cluster_figure.e#newcode202

view/eg_cluster_figure.e:202: if attached world as al_world then We prefer to use just `l_world'.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure.e

File view/eg_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure.e#oldcode41

view/eg_figure.e:41: l_model: like model I've looked at all descendants of EG_FIGURE in the library and EiffelStudio (haven't done so with the graph example), it seems that a figure is always created with a model, so making it attached, and promoting `make_with_model' instead of `default_create' would be beneficial to this class and descendants.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_factory.e

File view/eg_figure_factory.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_factory.e#oldcode85

view/eg_figure_factory.e:85: l_world: like world Let's change the code so that the creation procedure sets the world to begin with, or if this is not possible, create an empty world.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e

File view/eg_figure_world.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode70

view/eg_figure_world.e:70: make_with_model (a_model: like model) Why not making `model' attached? I see this is because if `make_filled'

that we need to keep because `new_filled_list' is calling it, let's change the definition so that it calls `make_with_model' too.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode1078

view/eg_figure_world.e:1078: l_rect := multi_select_rectangle Remove `is_multiselection_mode' by an object test on `multi_select_rectangle' instead.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode1110

view/eg_figure_world.e:1110: check l_rect /= Void end -- Implied by `is_multiselection_mode'

Same here

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode1144

view/eg_figure_world.e:1144: check l_rect /= Void end -- Implied by `is_multiselection_mode'

Same here.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_link_figure.e

File view/eg_link_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_link_figure.e#oldcode32

view/eg_link_figure.e:32: check l_model /= Void end -- Implied by precondition `model_not_void'

Force callers to call `make_with_model' to avoid checking for model that is always attached in practice.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_linkable_figure.e

File view/eg_linkable_figure.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_linkable_figure.e#newcode300

view/eg_linkable_figure.e:300: check attached world as al_world then -- Implied by precondition `ready'

Would be nice to make `world' attached all the time, but this could prove a little bit more difficult. But in any case, no need to use `check', here, just if is sufficient and in the else part a `check False end'.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e

File view/eg_polyline_link_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e#oldcode542

view/eg_polyline_link_figure.e:542: l_target := target If we can make `source' and `target' attached as mentionned before, that would simplify the code a lot.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e

File view/eg_polyline_link_figure.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e#newcode55

view/eg_polyline_link_figure.e:55: check attached model as al_model then

-- FIXME: Implied by ...?

See before, we can make `model' attached.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_resizable_cluster_figure.e

File view/eg_resizable_cluster_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_resizable_cluster_figure.e#oldcode137

view/eg_resizable_cluster_figure.e:137: if xml_routines.xml_boolean (node, is_user_sized_string) then I don't like the code here, we know that it is a boolean value, but then we need to check again. Maybe the xml parser should return a boolean value object that we can use to get a string from.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e

File view/eg_simple_link.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e#oldcode54

view/eg_simple_link.e:54: prune_all (line) We can simply use an object test on `reflexive' and have an invariant that says: model.is_reflexive = (reflexive /= Void)

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e

File view/eg_simple_link.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e#newcode139

view/eg_simple_link.e:139: check attached reflexive as al_reflexive then

-- FIXME: Implied by ...?

As above, let's use an object test.

 

http://patchreview.eiffel.com/5673173882241024/ 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/model/eg_linkable.e

File model/eg_linkable.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/model/eg_linkable.e#newcode33

model/eg_linkable.e:33: check attached name as al_name then Actually looking at the code, I don't think we ever verify that `name'

is not Void before using it, so I think we should make this query detachable, or provide a default name if none is set.

 

It is only used by the XML code to generate a valid XML code, but the name comes from the XML to begin with.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_force_directed_layout_n2.e

File physics/eg_force_directed_layout_n2.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_force_directed_layout_n2.e#newcode258

physics/eg_force_directed_layout_n2.e:258: check l_other /= Void then --

FIXME: Implied by...?

It might be more changes but I would recommend removing that and change EG_LINK_FIGURE to setup both `source' and `target' so that they are never Void. In practice, this is what happens since we always create descendants of EG_LINK_FIGURE with a model EG_LINK for which they are never Void.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation.e

File physics/eg_particle_simulation.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation.e#newcode107

physics/eg_particle_simulation.e:107: check l_result /= Void then -- Satisfy void safe compiler Actually here this is for typing only, so the whole body should be replaced by just:

 

    -- Typing only

check False then end

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e

File physics/eg_particle_simulation_bh.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e#oldcode105

physics/eg_particle_simulation_bh.e:105: check l_result /= Void end -- Implied by previsous if cluase and postcondition of `traverse_tree'

I would change the creation procedure of this class to create `quad_tree' and make `quad_tree' attached. That way most of the code doesn't need checks.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e

File physics/eg_particle_simulation_bh.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_bh.e#newcode127

physics/eg_particle_simulation_bh.e:127: check attached node.particle as al_particle then -- Implied by `is_leaf'

This is annoying, I would have 2 EG_QUAD_TREEs: EG_QUAD_TREE (where the

4 sub elements are attached and no particle) and EG_QUAD_TREE_LEAF that has a particle which is attached.

 

It might change the way we build the quad tree but seems easier. Now instead of doing node.is_leaf, we just do an object test attached {EG_QUAD_TREE_LEAF} node as l_leaf.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_n2.e

File physics/eg_particle_simulation_n2.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_particle_simulation_n2.e#oldcode37

physics/eg_particle_simulation_n2.e:37: Result := l_result I would change the code but since the class has been made obsolete for a long time now and is not in use for us, I would remove the class altogether.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_quad_tree.e

File physics/eg_quad_tree.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_quad_tree.e#oldcode53

physics/eg_quad_tree.e:53: check l_particle /= Void end -- Implied by invariant `leaf_has_particle_inner_nodes_do_not'

See my previous comment about rewritting EG_QUAD_TREE to avoid too much checks by splitting the class in 2 descendants.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_energy.e

File physics/eg_spring_energy.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_energy.e#oldcode102

physics/eg_spring_energy.e:102: check l_result /= Void end -- Satisfy void-safe compiler Use `check False then end' since this is for typing purpose only. Forgot to say it at the previous occurrence, but we should add a precondition `require False' as well, since this is not made to be called.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_particle.e

File physics/eg_spring_particle.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_spring_particle.e#oldcode121

physics/eg_spring_particle.e:121: Result := l_result See other comments about queries used for just typing purpose.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e

File physics/eg_vector2d.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/physics/eg_vector2d.e#newcode43

physics/eg_vector2d.e:43: check attached x as al_x then -- Implied by precondition `set'

We actually need to check callers of those and remove this. It was a bad initial void-safety conversion that is the cause of this code.

 

This is even worse, when you look at the caller such as `zero' there is no precondition to prevent that at all.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_circle_layout.e

File view/eg_circle_layout.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_circle_layout.e#newcode110

view/eg_circle_layout.e:110: check l_cluster /= Void then -- FIXME:

Implied by ...?

This is a command, so I would simply write:

if l_cluster /= Void then

  ... the line of today

else

   check cluster_preset: False end

end

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_cluster_figure.e

File view/eg_cluster_figure.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_cluster_figure.e#newcode202

view/eg_cluster_figure.e:202: if attached world as al_world then We prefer to use just `l_world'.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure.e

File view/eg_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure.e#oldcode41

view/eg_figure.e:41: l_model: like model I've looked at all descendants of EG_FIGURE in the library and EiffelStudio (haven't done so with the graph example), it seems that a figure is always created with a model, so making it attached, and promoting `make_with_model' instead of `default_create' would be beneficial to this class and descendants.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_factory.e

File view/eg_figure_factory.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_factory.e#oldcode85

view/eg_figure_factory.e:85: l_world: like world Let's change the code so that the creation procedure sets the world to begin with, or if this is not possible, create an empty world.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e

File view/eg_figure_world.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode70

view/eg_figure_world.e:70: make_with_model (a_model: like model) Why not making `model' attached? I see this is because if `make_filled'

that we need to keep because `new_filled_list' is calling it, let's change the definition so that it calls `make_with_model' too.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode1078

view/eg_figure_world.e:1078: l_rect := multi_select_rectangle Remove `is_multiselection_mode' by an object test on `multi_select_rectangle' instead.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode1110

view/eg_figure_world.e:1110: check l_rect /= Void end -- Implied by `is_multiselection_mode'

Same here

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_figure_world.e#oldcode1144

view/eg_figure_world.e:1144: check l_rect /= Void end -- Implied by `is_multiselection_mode'

Same here.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_link_figure.e

File view/eg_link_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_link_figure.e#oldcode32

view/eg_link_figure.e:32: check l_model /= Void end -- Implied by precondition `model_not_void'

Force callers to call `make_with_model' to avoid checking for model that is always attached in practice.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_linkable_figure.e

File view/eg_linkable_figure.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_linkable_figure.e#newcode300

view/eg_linkable_figure.e:300: check attached world as al_world then -- Implied by precondition `ready'

Would be nice to make `world' attached all the time, but this could prove a little bit more difficult. But in any case, no need to use `check', here, just if is sufficient and in the else part a `check False end'.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e

File view/eg_polyline_link_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e#oldcode542

view/eg_polyline_link_figure.e:542: l_target := target If we can make `source' and `target' attached as mentionned before, that would simplify the code a lot.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e

File view/eg_polyline_link_figure.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_polyline_link_figure.e#newcode55

view/eg_polyline_link_figure.e:55: check attached model as al_model then

-- FIXME: Implied by ...?

See before, we can make `model' attached.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_resizable_cluster_figure.e

File view/eg_resizable_cluster_figure.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_resizable_cluster_figure.e#oldcode137

view/eg_resizable_cluster_figure.e:137: if xml_routines.xml_boolean (node, is_user_sized_string) then I don't like the code here, we know that it is a boolean value, but then we need to check again. Maybe the xml parser should return a boolean value object that we can use to get a string from.

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e

File view/eg_simple_link.e (left):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e#oldcode54

view/eg_simple_link.e:54: prune_all (line) We can simply use an object test on `reflexive' and have an invariant that says: model.is_reflexive = (reflexive /= Void)

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e

File view/eg_simple_link.e (right):

 

http://patchreview.eiffel.com/5673173882241024/diff/5629499534213120/view/eg_simple_link.e#newcode139

view/eg_simple_link.e:139: check attached reflexive as al_reflexive then

-- FIXME: Implied by ...?

As above, let's use an object test.

 

http://patchreview.eiffel.com/5673173882241024/

[Emmanuel Stapf]

Just a quick reference to the small tutorial I wrote about void-safety conversion that I'm sure will help in migrating to void-safety:

http://eiffelroom.org/article/how_to_convert_your_code_to_be_voidsafe

Manu

[Anders Persson]

HI

Is this an alternative?

http://www.reviewboard.org/

Seems to have CVS support at least. However, it is no free.

Regards 

Anders

--
For more messaging options, visit this group at http://forum.eiffel.com.
Information on the Eiffelstudio project: http://dev.eiffel.com.

[Jocelyn Fiat]

I would suggest to use https://codereview.appspot.com/

This is the same application as used for patchreview.eiffel.com but this is public.

-- Jocelyn

--
Jocelyn
------------------------------------------------------------------------
Eiffel Software
805-685-1006
http://www.eiffel.com
Customer support: http://support.eiffel.com
User group: http://groups.eiffel.com/join
------------------------------------------------------------------------

[Emmanuel Stapf]

Unfortunately it doesn’t work as it requires you to have python installed to upload patch L

 

Manu

[Jocelyn Fiat]

Indeed, this is a recent change ...

and it may be possible that the same change applies soon or later on patchreview.eiffel.com ...

Otherwise I tried to install http://www.reviewboard.org/ on a linux machine, and so far without success.

http://phabricator.org/ is also another alternative.

To be continued...

[Anders Persson]

Hi

I have made most changes to the Memory_analyzer library. I have some issues to discuss that probably are best done in a code review where I can easily refer to the code. The main problems that I want to discuss is related to unclear preconditions that might exists but is difficult to pinpoint if you do not have good knowledge of the library.

I have tried to read the review notes from the graph library review but do not understand much of it.

Regards

Anders

[Emmanuel Stapf]

Jocelyn and I are trying reviewboard. It is not great, but at least we could use it as a basis for the review. I hope that early next week we will have something.

 

In the meantime, do not hesitate to use the forum to discuss issues, but let’s do one at a time, otherwise it is hard to follow.

 

Thanks,

Manu

[Anders Persson]

Okej.  Then we start with the one I wrote some days ago

Regards

Anders

[Anders Persson]

Hi

How is it going with the reviewboard?

Should we try with just sending some diffs as you suggested to David?

Regards

Anders

...