Complete Void-safety review for graph library

Complete Void-safety review for graph library

[Manus, 2014-04-02 18:22:00]

Here is the review. In addition check the following things that could simplify
the code:

- Make `model' attached in all the figures.
- Better contracts around `source' and `target' in EG_LINK_FIGURE that could
simplify some code or write the code differently to avoid object tests on them.

Also make sure to read:

http://eiffelroom.org/article/how_to_convert_your_code_to_be_voidsafe

to avoid `check ... then end' as much as possible. This is just a way to make
the compiler happy, not a way to make your code truly void-safe.

Thanks,
Manu

https://codereview.appspot.com/83240043/diff/1/model/eg_item.e
File model/eg_item.e (right):

https://codereview.appspot.com/83240043/diff/1/model/eg_item.e#newcode34
model/eg_item.e:34: -- Unique id
Keep the dot.

https://codereview.appspot.com/83240043/diff/1/model/eg_linkable.e
File model/eg_linkable.e (right):

https://codereview.appspot.com/83240043/diff/1/model/eg_linkable.e#newcode33
model/eg_linkable.e:33: check attached name as l_name then
I would rather have `link_name' be detachable.

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
File physics/eg_force_directed_layout_n2.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
physics/eg_force_directed_layout_n2.e:256: check l_other /= Void then -- FIXME:
Implied by...?
Can we have `a_edge.target' and `a_edge.source' statically attached and thus
avoid the check.

If this is not possible, then maybe using `if' is ok since it just means that it
has no effect on `l_item'.

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
physics/eg_force_directed_layout_n2.e:337: check l_other /= Void then -- FIXME:
Implied by ...?
See above.

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
File physics/eg_particle_simulation_bh.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_bh.e:116: Result := n_body_force (a_particle,
l_particle)
Instead of the comment, add in the compound of the if branch:

check is_leaf: node.is_leaf end

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_bh.e:118: check attached
node.center_of_mass_particle as l_cmp then -- FIXME Implied by...
Reading the note clause of the class, we see that for the traversal there are 3
cases:
- leaf
- what is currently done now
- all of the above.

Also looking at the code `cmp' is only used to calculate `r' and `prop' and cmp
is used again just for `prop < theta'. So I suggest that we do:

if attached node.center_of_mass_particle as l_cmp then
	region := node.region
		-- Distance to center of mass
	r := distance (a_particle.x, a_particle.y, cmp.x, cmp.y)
		-- size of the cell
	d := region.width.max (region.height)
		-- proportion between distance and size
	prop := d/r

	if prop < 1.0 then
		last_theta_average := last_theta_average + prop
		theta_count := theta_count + 1
	end
	if prop < theta then
			-- Approximate
		l_result := n_body_force (a_particle, cmp)
	else
		l_is_default_case := True
	end
else
	l_is_default_case := True
end

if l_is_default_case then
	-- We put the #3 case above traversing all children.
end

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_bh.e:204: check l_quad_tree /= Void then --
Implied by previous loop
I do not like this. Reading the code it seems clear that if `particles' is not
empty we could take the first one and create the quad_tree from there, and then
traverse the remaining items.

And we have that invariant about `particles'. So I would do that rather than
having a check.

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
File physics/eg_particle_simulation_n2.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_n2.e:36: check l_result /= Void then -- FIXME:
Implied by ...
We know that particles is not empty per invariant. So I would take the first
item of the list to initialize `Result' and then traverse the remaining items.

Thus removing the need for `l_result' and a check .. end statement.

https://codereview.appspot.com/83240043/diff/1/physics/eg_quad_tree.e
File physics/eg_quad_tree.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_quad_tree.e#newcode51
physics/eg_quad_tree.e:51: if is_leaf then
Since we have the invariant, we should write:

if attached particle as l_particle then
   check is_leaf: is_leaf
   Result := .....
else
   ....

https://codereview.appspot.com/83240043/diff/1/physics/eg_quad_tree.e#newcode110
physics/eg_quad_tree.e:110: l_particle := l_childe_sw.center_of_mass_particle
I do not like check :-) as you may have noticed by now.

I wonder if we should drop the `build_center_of_mass' command have have instead
`center_of_mass_particle' being a function that returns a stored value by
default unless reset by a call to `reset_center_of_mass'. That way the code just
becomes:

if attached childe_sw as l_childe_sw then
    l_particle := l_childe_sw.center_of_mass_particle
    mass := l_particle.mass
    ....
end

and no need to worry about void-safety here.

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_energy.e
File physics/eg_spring_energy.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_energy.e#new...
physics/eg_spring_energy.e:101: check anchor_type_only: False end
Just use a precondition `not_callable: False' and add `check False then end' in
body

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e
File physics/eg_spring_particle.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e#n...
physics/eg_spring_particle.e:84: check l_other /= Void then -- FXIME: Implied by
...?
See previous note about making target and source attached by default?

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e#n...
physics/eg_spring_particle.e:120: check anchor_type_only: False end
Add precondition `not_callable' if not present already and `check False then
end' in body.

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e
File physics/eg_vector2d.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode38
physics/eg_vector2d.e:38: attached_x: G
We should never use them. Can you check all callers and update them so that they
check that `x', `y' are attached instead of using `attached_x' or `attached_y'?

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode112
physics/eg_vector2d.e:112: check l_result /= Void then -- Satisfy void-safe
compiler
Use `check not_implemented: False then end'

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode123
physics/eg_vector2d.e:123: check l_result /= Void then -- Satisfy void-safe
compiler
Use `check not_implemented: False then end'

https://codereview.appspot.com/83240043/diff/1/view/eg_circle_layout.e
File view/eg_circle_layout.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_circle_layout.e#newcod...
view/eg_circle_layout.e:110: check l_cluster /= Void then -- FIXME: Implied by
...?
Should we just do nothing if `l_link' has no cluster?

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e
File view/eg_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode41
view/eg_figure.e:41: check attached model as l_model then -- Implied by
precondition `model_not_void'
Should we make `model' attached? I think this is possible because all
descendants seems to create figures using a model.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode78
view/eg_figure.e:78: check attached model as l_model then -- FIXME: Implied by
...?
To remove if `model' is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode102
view/eg_figure.e:102: if node.has_attribute_by_name (name_string) then
Use an object test on:

if attached node.attribute_by_name (name_string) as l_attribute then
   ...
end

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode104
view/eg_figure.e:104: check l_attribute /= Void then -- Implied by
`has_attribute_by_name'
To remove with the above object test.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode107
view/eg_figure.e:107: check attached model as l_model then -- FIXME: Implied by
...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_factory.e
File view/eg_figure_factory.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_factory.e#newco...
view/eg_figure_factory.e:86: check attached world as l_world then -- Implied by
precondition `world_not_void'
Since `linkable_with_name' returns a detachable type, I would drop the
precondition `world_not_void' and just use a `if' in the body.

which means that if there is no world, there is no linkable with a give name.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e
File view/eg_figure_world.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode215
view/eg_figure_world.e:215: check attached model as l_model then -- Implied by
precondition `set'
Made model attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode230
view/eg_figure_world.e:230: check attached factory as l_factory then -- Implied
by precondition `set'
Remove attached_factory and update callers accordingly.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode524
view/eg_figure_world.e:524: check l_model /= Void then -- FIXME: Implied by ...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode...
view/eg_figure_world.e:1075: check attached multi_select_rectangle as al_rect
then -- Implied by `is_multiselection_mode'
I would remove `is_multiselection_mode' and use `if attached
multi_select_rectangle as l_rect' instead.

To make it work, in the _release' version we need to set
`multi_select_rectangle' to Void.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode...
view/eg_figure_world.e:1104: if is_multiselection_mode then
Use `if attached multi_select_rectange as l_rect' instead.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode...
view/eg_figure_world.e:1137: check attached multi_select_rectangle as l_rect
then -- Implied by `is_multiselection_mode'
Use if attached multi_select_rectangle as l_rect then instead.

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e
File view/eg_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode29
view/eg_link_figure.e:29: check attached model as l_model then -- Implied by
precondition `model_not_void'
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode61
view/eg_link_figure.e:61: check l_model /= Void then -- FIXME: Implied by ...?
To remove is model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode74
view/eg_link_figure.e:74: check attached model as l_model then -- FIXME: Implied
by ...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode138
view/eg_link_figure.e:138: copyright:	"Copyright (c) 1984-2014, Eiffel Software
and others"
Can we add some invariants to know when `source' or `target' are attached?

https://codereview.appspot.com/83240043/diff/1/view/eg_linkable_figure.e
File view/eg_linkable_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_linkable_figure.e#newc...
view/eg_linkable_figure.e:300: check attached world as l_world then -- Implied
by precondition `ready'
Since there is a precondition, no need to do the `check' Just a if is sufficient
and add an else part with `check world_attached: False end -- Per precondition'.

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e
File view/eg_polyline_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:55: check attached model as l_model then --
FIXME: Implied by ...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:440: check attached model as l_model then
--FIXME: Implied by ...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:519: check attached source as l_source then --
FIXME: Implied by ...?
I do not like this but I don't have a suggestion for the moment. Same for
`set_end_point_to_edge' and `set_end_and_start_point_to_edge'.

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:634: check attached model as l_model then --
FIXME: Implied by ...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_resizable_cluster_figu...
File view/eg_resizable_cluster_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_resizable_cluster_figu...
view/eg_resizable_cluster_figure.e:137: if xml_routines.xml_boolean (node,
is_user_sized_string) then
Use object test as

if attached xml_routines.xml_string (node, user_size_string) as l_str and then
l_str.is_boolean then
   user_size := rectangle_from_string (l_str)
end

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e
File view/eg_simple_link.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode53
view/eg_simple_link.e:53: check attached reflexive as l_reflexive then --
Implied by `is_reflexive'
`reflexive' being created all the time in EG_SIMPLE_LINK.default_Create, the
attribute should drop its `detachable status.

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode108
view/eg_simple_link.e:108: check attached model as l_model then -- FIXME:
Implied by ...?
To remove if model is attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode139
view/eg_simple_link.e:139: check attached reflexive as l_reflexive then --
Implied by invariant `reflexive_model_equivalence'
To remove since reflexive is always attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode144
view/eg_simple_link.e:144: check attached reflexive as l_reflexive then --
Implied by invariant `reflexive_model_equivalence'
To remove since reflexive is always attached.

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode170
view/eg_simple_link.e:170: check attached model as l_model then -- FIXME:
Implied by ...?
To remove if model is attached.

[Conaclos, 2014-04-05 16:14:28]

https://codereview.appspot.com/83240043/diff/1/model/eg_item.e
File model/eg_item.e (right):

https://codereview.appspot.com/83240043/diff/1/model/eg_item.e#newcode34
model/eg_item.e:34: -- Unique id
On 2014/04/02 18:22:00, Manus wrote:
> Keep the dot.

Done.

https://codereview.appspot.com/83240043/diff/1/model/eg_linkable.e
File model/eg_linkable.e (right):

https://codereview.appspot.com/83240043/diff/1/model/eg_linkable.e#newcode33
model/eg_linkable.e:33: check attached name as l_name then
I will add this change in the second patch

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
File physics/eg_force_directed_layout_n2.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
physics/eg_force_directed_layout_n2.e:256: check l_other /= Void then -- FIXME:
Implied by...?
For `target' and `source' see next comment.

If the attached status of both queries is changed, it will be performed in the
second patch since it is an important interface change.
For this first patch I will replace `check' with `if'.

Note: This class is obsolete.

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
physics/eg_force_directed_layout_n2.e:256: check l_other /= Void then -- FIXME:
Implied by...?
On 2014/04/02 18:22:00, Manus wrote:
> Can we have `a_edge.target' and `a_edge.source' statically attached and thus
> avoid the check.
> 
> If this is not possible, then maybe using `if' is ok since it just means that
it
> has no effect on `l_item'.

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_force_directed_layo...
physics/eg_force_directed_layout_n2.e:337: check l_other /= Void then -- FIXME:
Implied by ...?
On 2014/04/02 18:22:00, Manus wrote:
> See above.

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
File physics/eg_particle_simulation_bh.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_bh.e:116: Result := n_body_force (a_particle,
l_particle)
On 2014/04/02 18:22:00, Manus wrote:
> Instead of the comment, add in the compound of the if branch:
> 
> check is_leaf: node.is_leaf end

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_bh.e:118: check attached
node.center_of_mass_particle as l_cmp then -- FIXME Implied by...
Yes It is a good suggestion.
In order to avoid the addition of a new check statment,
i.e

if l_is_default_case then
	-- We put the #3 case above traversing all children.
else
	cehck l_result /= Void then
		Result := l_result
	end
end

I suggest to use  `l_result' instead of `l_is_default_case'.
i.e

if l_result /= Void then
	Result := l_result
else
	-- We put the #3 case above traversing all children.
end

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_bh.e:204: check l_quad_tree /= Void then --
Implied by previous loop
On 2014/04/02 18:22:00, Manus wrote:
> I do not like this. Reading the code it seems clear that if `particles' is not
> empty we could take the first one and create the quad_tree from there, and
then
> traverse the remaining items.
> 
> And we have that invariant about `particles'. So I would do that rather than
> having a check.
> 

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
File physics/eg_particle_simulation_n2.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_particle_simulation...
physics/eg_particle_simulation_n2.e:36: check l_result /= Void then -- FIXME:
Implied by ...
I didn't redesign and justify it because I made obsolete this class in the
redesign patch proposal.

However I acepted it since patches should be independnats.

https://codereview.appspot.com/83240043/diff/1/physics/eg_quad_tree.e
File physics/eg_quad_tree.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_quad_tree.e#newcode51
physics/eg_quad_tree.e:51: if is_leaf then
On 2014/04/02 18:22:00, Manus wrote:
> Since we have the invariant, we should write:
> 
> if attached particle as l_particle then
>    check is_leaf: is_leaf
>    Result := .....
> else
>    ....

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_quad_tree.e#newcode110
physics/eg_quad_tree.e:110: l_particle := l_childe_sw.center_of_mass_particle
The change will be introduced in the second patch.

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_energy.e
File physics/eg_spring_energy.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_energy.e#new...
physics/eg_spring_energy.e:101: check anchor_type_only: False end
On 2014/04/02 18:22:00, Manus wrote:
> Just use a precondition `not_callable: False' and add `check False then end'
in
> body

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e
File physics/eg_spring_particle.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e#n...
physics/eg_spring_particle.e:84: check l_other /= Void then -- FXIME: Implied by
...?
I thought about this change.
The interface improving is important, however there is sensitive change.
We could either change the creation procedure implementation or else the
creation procedure signature.
i.e.
make_with_model (a_model: like model; a_source, a_target: like source)

The first choice implied to create `source' and `traget' with a default
abstraction, maybe EG_NODE?

The second choice implied to change supplier code and t ochange the signature of
{EG_SIMPLE_FACTORY}.new_link_figure.
i.e.
new_link_figure (a_link: EG_LINK; a_source, a_target: EG_LINKABLE_FIGURE)


Have you a preference as "client" of the graph library?

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e#n...
physics/eg_spring_particle.e:120: check anchor_type_only: False end
On 2014/04/02 18:22:00, Manus wrote:
> Add precondition `not_callable' if not present already and `check False then
> end' in body.

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e
File physics/eg_vector2d.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode38
physics/eg_vector2d.e:38: attached_x: G
Since it is a sensitive interface change it is part of the redesign (second)
patch.
In the second patch these features are not removed but deprecated.

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode112
physics/eg_vector2d.e:112: check l_result /= Void then -- Satisfy void-safe
compiler
On 2014/04/02 18:22:00, Manus wrote:
> Use `check not_implemented: False then end'

It is part of the redesign patch.
However since it is not change the interface it is more logic to integrate this
change directly in the current patch.

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode112
physics/eg_vector2d.e:112: check l_result /= Void then -- Satisfy void-safe
compiler
On 2014/04/02 18:22:00, Manus wrote:
> Use `check not_implemented: False then end'

Done.

https://codereview.appspot.com/83240043/diff/1/physics/eg_vector2d.e#newcode123
physics/eg_vector2d.e:123: check l_result /= Void then -- Satisfy void-safe
compiler
On 2014/04/02 18:22:00, Manus wrote:
> Use `check not_implemented: False then end' 

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_circle_layout.e
File view/eg_circle_layout.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_circle_layout.e#newcod...
view/eg_circle_layout.e:110: check l_cluster /= Void then -- FIXME: Implied by
...?
On 2014/04/02 18:22:00, Manus wrote:
> Should we just do nothing if `l_link' has no cluster?

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e
File view/eg_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode41
view/eg_figure.e:41: check attached model as l_model then -- Implied by
precondition `model_not_void'
This change was performed in the second patch since it can make incompatible a
large amount of software (including Eiffel Studio) using the Graph Library.

Note: Memory Analyzer library integrate necessary change to be compatible with
both patches.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode78
view/eg_figure.e:78: check attached model as l_model then -- FIXME: Implied by
...?
See above

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode102
view/eg_figure.e:102: if node.has_attribute_by_name (name_string) then
On 2014/04/02 18:22:00, Manus wrote:
> Use an object test on:
> 
> if attached node.attribute_by_name (name_string) as l_attribute then
>    ...
> end

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode104
view/eg_figure.e:104: check l_attribute /= Void then -- Implied by
`has_attribute_by_name'
On 2014/04/02 18:22:00, Manus wrote:
> To remove with the above object test.

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure.e#newcode107
view/eg_figure.e:107: check attached model as l_model then -- FIXME: Implied by
...?
See above

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_factory.e
File view/eg_figure_factory.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_factory.e#newco...
view/eg_figure_factory.e:86: check attached world as l_world then -- Implied by
precondition `world_not_void'
On 2014/04/02 18:22:00, Manus wrote:
> Since `linkable_with_name' returns a detachable type, I would drop the
> precondition `world_not_void' and just use a `if' in the body.
> 
> which means that if there is no world, there is no linkable with a give name.

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e
File view/eg_figure_world.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode215
view/eg_figure_world.e:215: check attached model as l_model then -- Implied by
precondition `set'
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode230
view/eg_figure_world.e:230: check attached factory as l_factory then -- Implied
by precondition `set'
This change is part of redesign patch since it is an interface change.

I made the feature obsolete.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode524
view/eg_figure_world.e:524: check l_model /= Void then -- FIXME: Implied by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode...
view/eg_figure_world.e:1075: check attached multi_select_rectangle as al_rect
then -- Implied by `is_multiselection_mode'
On 2014/04/02 18:22:00, Manus wrote:
> I would remove `is_multiselection_mode' and use `if attached
> multi_select_rectangle as l_rect' instead.
> 
> To make it work, in the _release' version we need to set
> `multi_select_rectangle' to Void.

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode...
view/eg_figure_world.e:1104: if is_multiselection_mode then
On 2014/04/02 18:22:00, Manus wrote:
> Use `if attached multi_select_rectange as l_rect' instead.

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_figure_world.e#newcode...
view/eg_figure_world.e:1137: check attached multi_select_rectangle as l_rect
then -- Implied by `is_multiselection_mode'
On 2014/04/02 18:22:00, Manus wrote:
> Use if attached multi_select_rectangle as l_rect then instead.

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e
File view/eg_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode29
view/eg_link_figure.e:29: check attached model as l_model then -- Implied by
precondition `model_not_void'
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode61
view/eg_link_figure.e:61: check l_model /= Void then -- FIXME: Implied by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode74
view/eg_link_figure.e:74: check attached model as l_model then -- FIXME: Implied
by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_link_figure.e#newcode138
view/eg_link_figure.e:138: copyright:	"Copyright (c) 1984-2014, Eiffel Software
and others"
If we make `source' and `target' attached, it is not useful.

https://codereview.appspot.com/83240043/diff/1/view/eg_linkable_figure.e
File view/eg_linkable_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_linkable_figure.e#newc...
view/eg_linkable_figure.e:300: check attached world as l_world then -- Implied
by precondition `ready'

Could you reply to my question at
http://eiffelroom.org/article/how_to_convert_your_code_to_be_voidsafe

I don't understand why this modification is relevant...

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e
File view/eg_polyline_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:55: check attached model as l_model then --
FIXME: Implied by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:440: check attached model as l_model then
--FIXME: Implied by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:519: check attached source as l_source then --
FIXME: Implied by ...?
I added precondition in the second patch `source_attached' and
`target_attached'.

These three routines are only internally called.

https://codereview.appspot.com/83240043/diff/1/view/eg_polyline_link_figure.e...
view/eg_polyline_link_figure.e:634: check attached model as l_model then --
FIXME: Implied by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_resizable_cluster_figu...
File view/eg_resizable_cluster_figure.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_resizable_cluster_figu...
view/eg_resizable_cluster_figure.e:137: if xml_routines.xml_boolean (node,
is_user_sized_string) then
On 2014/04/02 18:22:00, Manus wrote:
> Use object test as
> 
> if attached xml_routines.xml_string (node, user_size_string) as l_str and then
> l_str.is_boolean then
>    user_size := rectangle_from_string (l_str)
> end

Done.

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e
File view/eg_simple_link.e (right):

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode53
view/eg_simple_link.e:53: check attached reflexive as l_reflexive then --
Implied by `is_reflexive'
I will add this change in the second patch.

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode108
view/eg_simple_link.e:108: check attached model as l_model then -- FIXME:
Implied by ...?
See comment in `eg_figure.e'

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode139
view/eg_simple_link.e:139: check attached reflexive as l_reflexive then --
Implied by invariant `reflexive_model_equivalence'
See above

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode144
view/eg_simple_link.e:144: check attached reflexive as l_reflexive then --
Implied by invariant `reflexive_model_equivalence'
See above

https://codereview.appspot.com/83240043/diff/1/view/eg_simple_link.e#newcode170
view/eg_simple_link.e:170: check attached model as l_model then -- FIXME:
Implied by ...?
See comment in `eg_figure.e'

[Conaclos, 2014-04-10 19:06:58]

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e
File physics/eg_spring_particle.e (right):

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e#n...
physics/eg_spring_particle.e:84: check l_other /= Void then -- FXIME: Implied by
...?
I decided to impement the second choice in the second patch.

https://codereview.appspot.com/83240043/diff/1/physics/eg_spring_particle.e#n...
physics/eg_spring_particle.e:84: check l_other /= Void then -- FXIME: Implied by
...?
On 2014/04/02 18:22:00, Manus wrote:
> See previous note about making target and source attached by default?

Done.

[Manus, 2014-04-15 17:05:46]

I just put a few minor comments. I suggest that to upload the other set of
changes and that we will continue the code review on those from now on.

https://codereview.appspot.com/83240043/diff/20001/physics/eg_particle_simula...
File physics/eg_particle_simulation_bh.e (right):

https://codereview.appspot.com/83240043/diff/20001/physics/eg_particle_simula...
physics/eg_particle_simulation_bh.e:165: if l_result = Void then
I think we need to add an invariant that states that if it is not a leaf, then
one of the `child_xx' query is attached. That way the check makes sense.

https://codereview.appspot.com/83240043/diff/20001/view/eg_linkable_figure.e
File view/eg_linkable_figure.e (right):

https://codereview.appspot.com/83240043/diff/20001/view/eg_linkable_figure.e#...
view/eg_linkable_figure.e:301: if a_cluster = Void then
I had missed this question in eiffelroom. I've just answered.

https://codereview.appspot.com/83240043/diff/20001/view/eg_polyline_link_figu...
File view/eg_polyline_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/20001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:521: check source /= Void then -- Implied by
`source_attached'
Use `if source /= Void then' instead thanks to the precondition.

https://codereview.appspot.com/83240043/diff/20001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:541: check target /= Void then -- Implied by
`target_attached'
Use `if target /= Void then' instead thanks to the precondition.

https://codereview.appspot.com/83240043/diff/20001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:558: check source /= Void and target /= Void then
-- Implied by `source_attached' and `target_attached'
Use `if' instead thanks to the preconditions

[Manus, 2014-04-15 18:25:09]

Thanks for the work. The library looks much nicer this way. Go through the
comments. My only concern is the removal of `make_filled' and its associated
redefinition of `new_filled_list'. I'm not even sure to see why it compiles (not
that I haven't compiled the code, I just did a review based on the patch).

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e
File model/eg_cluster.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode38
model/eg_cluster.e:38: Result := it.item = a_linkable
There is a bug here, you should add `until Result' to preserve the original
behavior.

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode116
model/eg_cluster.e:116: -- a linkable was removed from `Current'
Add dot again.

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode137
model/eg_cluster.e:137: l_cluster.prune_all (l_cluster)
I think there is a bug here, we should keep `a_linkable' as argument, no?

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode177
model/eg_cluster.e:177: -- Anchor type.
Add precondition `require callable: False' as recommended for those kind of
routines

https://codereview.appspot.com/83240043/diff/40001/model/eg_graph.e
File model/eg_graph.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_graph.e#newcode329
model/eg_graph.e:329: do
Add precondition

https://codereview.appspot.com/83240043/diff/40001/model/eg_item.e
File model/eg_item.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_item.e#newcode71
model/eg_item.e:71: ---		require
Remove commented code.

https://codereview.appspot.com/83240043/diff/40001/model/eg_node.e
File model/eg_node.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_node.e#newcode16
model/eg_node.e:16: 
Any reason for removing the clause, I prefer when things are explicit.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_force_directed_...
File physics/eg_force_directed_layout_n2.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_force_directed_...
physics/eg_force_directed_layout_n2.e:356: if a_node /= l_other and
l_other.is_show_requested then --and then l_item.has_visible_link then
I'm not sure about the commented `-- and then l_item.has_visible_link' but if
you cannot make sense of it, maybe we can simply remove it.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_quad_tree.e
File physics/eg_quad_tree.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_quad_tree.e#new...
physics/eg_quad_tree.e:99: if attached center_of_mass_particle_cache as l_cahe
then
Fixed typo on object test local

https://codereview.appspot.com/83240043/diff/40001/physics/eg_spring_particle.e
File physics/eg_spring_particle.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_spring_particle...
physics/eg_spring_particle.e:86: create Result.make (Result.x - l_weight * (px -
l_other.port_x), Result.y - l_weight * (py - l_other.port_y))
Do we need to create it as second time? Why not calling `set' as done before?
This is going to be a costly operation. In that case we might be better off
using 2 local variables and then create Result at the very end.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_spring_particle...
physics/eg_spring_particle.e:100: create Result.make (0.0, 0.0)
Is changing the order of compounds the result of using the static Analyzer?

https://codereview.appspot.com/83240043/diff/40001/physics/eg_vector2d.e
File physics/eg_vector2d.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_vector2d.e#newc...
physics/eg_vector2d.e:47: "Create a new instance with `make' creation procedure
since EG_VECTOR2D is immutable. [03-2014]"
Was there a reason for making EG_VECTOR2D immutable? If so, add a note clause to
the effect.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure.e
File view/eg_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure.e#newcode14
view/eg_figure.e:14: new_filled_list
I don't think there is a need to export it to NONE since it is already not
exported.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure.e#newcode96
view/eg_figure.e:96: model.set_name (l_attribute.value)
Looks like there might be a breaking change as {EG_ITEM}.set_name only performs
reference comparison not object comparison.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_factory.e
File view/eg_figure_factory.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_factory.e#n...
view/eg_figure_factory.e:55: -- Element type for compilation purpose.
Add precondition.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e
File view/eg_figure_world.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:35: make_with_model
I'm still puzzled at the removal of this code as if you call `duplicate' on
EG_FIGURE_WORLD, does it yield something correct?

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:87: a_factory.set_world (Current)
Probably we should make `set_factory' obsolete?

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:158: "Use `has_linkable_figure' instead; [04-2014]"
This sounds wrong to me since it is looking for a EG_CLUSTER_FIGURE, unless
there is somewhere a contract that says if we have a EG_CLUSTER then it can only
be a model of EG_CLUSTER_FIGURE, no?

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:435: not_has_a_link: not has_link_figure (a_link)
I liked the `has_source_figure' and `has_target_figure' contract. Is there
somewhere some contracts to imply this? I understand now that if it was not the
case then it doesn't do anything but it feels wrong.

I am under the assumption that if we have `model.has_link (a_link)' then it
implies that model.has_linkable (a_link.source) and model.has_linkable
(a_link.target). And by implication if the model has the source and target then
there should be some figures associated to those but I'm not sure where we can
get this from.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:1060: end
Should we just add a contract stating that both linkables_to_figures and
links_to_figures are unchanged?

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:1105: check not_implemented: False then end
See remark at the top.

https://codereview.appspot.com/83240043/diff/40001/view/eg_force_directed_lay...
File view/eg_force_directed_layout.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_force_directed_lay...
view/eg_force_directed_layout.e:261: l_linkables.new_cursor as it
No need to call `new_cursor'.

https://codereview.appspot.com/83240043/diff/40001/view/eg_link_figure.e
File view/eg_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_link_figure.e#newc...
view/eg_link_figure.e:73: if attached l_model.source.link_name as l_link_name
then
The generated XML might not make sense if we have neither or just one of SOURCE
and TARGET. Is the retrieval of the XML taking care of that?

https://codereview.appspot.com/83240043/diff/40001/view/eg_polyline_link_figu...
File view/eg_polyline_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:28: make
See previous comment for dropping `make_filled'.

https://codereview.appspot.com/83240043/diff/40001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:120: line_width: INTEGER
Why not adding a comment to all those queries?

https://codereview.appspot.com/83240043/diff/40001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:638: check not_implemented: False then end
See comment at the top of the class.

https://codereview.appspot.com/83240043/diff/40001/view/eg_resizable_cluster_...
File view/eg_resizable_cluster_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_resizable_cluster_...
view/eg_resizable_cluster_figure.e:77: left: INTEGER
Add comment.

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_cluster.e
File view/eg_simple_cluster.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_cluster.e#n...
view/eg_simple_cluster.e:21: 
See earlier comments on `make_filled'

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_cluster.e#n...
view/eg_simple_cluster.e:193: check not_implemented: False then end
See comment at the top.

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_factory.e
File view/eg_simple_factory.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_factory.e#n...
view/eg_simple_factory.e:45: attached node.attribute_by_name ("SOURCE") as
l_attribute_by_name and then
I see that now you check for both SOURCE and TARGET to be attached which is good
for a badly formatted XML case. Ignore my comment about the storing part.

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_link.e
File view/eg_simple_link.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_link.e#newc...
view/eg_simple_link.e:19: make
See other comments about `make_filled' and `new_filled_list'.

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_node.e
File view/eg_simple_node.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_node.e#newc...
view/eg_simple_node.e:21: make_with_model
See other comments about `make_filled' and `new_filled_list'.

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_node.e#newc...
view/eg_simple_node.e:40: create node_figure -- Satisfy invariant
I'm not sure if this comment is useful. Creations are always there to satisfy an
invariant.

[Conaclos, 2014-04-19 17:42:34]

https://codereview.appspot.com/83240043/diff/20001/physics/eg_particle_simula...
File physics/eg_particle_simulation_bh.e (right):

https://codereview.appspot.com/83240043/diff/20001/physics/eg_particle_simula...
physics/eg_particle_simulation_bh.e:165: if l_result = Void then
The invariant was added in the second patch.
I add it in the first patch.

leaf_has_no_childe: is_leaf = (childe_sw = Void and childe_se = Void and
childe_ne = Void and childe_nw = Void)

https://codereview.appspot.com/83240043/diff/20001/physics/eg_particle_simula...
physics/eg_particle_simulation_bh.e:165: if l_result = Void then
On 2014/04/15 17:05:46, Manus wrote:
> I think we need to add an invariant that states that if it is not a leaf, then
> one of the `child_xx' query is attached. That way the check makes sense.
>   

Done.

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e
File model/eg_cluster.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode38
model/eg_cluster.e:38: Result := it.item = a_linkable
On 2014/04/15 18:25:10, Manus wrote:
> There is a bug here, you should add `until Result' to preserve the original
> behavior.

Done.

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode116
model/eg_cluster.e:116: -- a linkable was removed from `Current'
On 2014/04/15 18:25:10, Manus wrote:
> Add dot again.

Done.

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode137
model/eg_cluster.e:137: l_cluster.prune_all (l_cluster)
On 2014/04/15 18:25:10, Manus wrote:
> I think there is a bug here, we should keep `a_linkable' as argument, no?

Done.

https://codereview.appspot.com/83240043/diff/40001/model/eg_cluster.e#newcode177
model/eg_cluster.e:177: -- Anchor type.
On 2014/04/15 18:25:10, Manus wrote:
> Add precondition `require callable: False' as recommended for those kind of
> routines

Done.

https://codereview.appspot.com/83240043/diff/40001/model/eg_graph.e
File model/eg_graph.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_graph.e#newcode329
model/eg_graph.e:329: do
On 2014/04/15 18:25:10, Manus wrote:
> Add precondition

Done.

https://codereview.appspot.com/83240043/diff/40001/model/eg_item.e
File model/eg_item.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_item.e#newcode71
model/eg_item.e:71: ---		require
Red lines seems removed lines ;)

https://codereview.appspot.com/83240043/diff/40001/model/eg_node.e
File model/eg_node.e (right):

https://codereview.appspot.com/83240043/diff/40001/model/eg_node.e#newcode16
model/eg_node.e:16: 
On 2014/04/15 18:25:10, Manus wrote:
> Any reason for removing the clause, I prefer when things are explicit.

Done.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_force_directed_...
File physics/eg_force_directed_layout_n2.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_force_directed_...
physics/eg_force_directed_layout_n2.e:356: if a_node /= l_other and
l_other.is_show_requested then --and then l_item.has_visible_link then
On 2014/04/15 18:25:10, Manus wrote:
> I'm not sure about the commented `-- and then l_item.has_visible_link' but if
> you cannot make sense of it, maybe we can simply remove it.

Done.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_quad_tree.e
File physics/eg_quad_tree.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_quad_tree.e#new...
physics/eg_quad_tree.e:99: if attached center_of_mass_particle_cache as l_cahe
then
On 2014/04/15 18:25:10, Manus wrote:
> Fixed typo on object test local

Done.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_quad_tree.e#new...
physics/eg_quad_tree.e:99: if attached center_of_mass_particle_cache as l_cahe
then
Note:
A commit was accidently not included in the second patch.
This last commit bring few changes in this routine (removing
`build_center_of_mass' and adding reset in few places).

https://codereview.appspot.com/83240043/diff/40001/physics/eg_spring_particle.e
File physics/eg_spring_particle.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_spring_particle...
physics/eg_spring_particle.e:86: create Result.make (Result.x - l_weight * (px -
l_other.port_x), Result.y - l_weight * (py - l_other.port_y))
On 2014/04/15 18:25:10, Manus wrote:
> Do we need to create it as second time? Why not calling `set' as done before?
> This is going to be a costly operation. In that case we might be better off
> using 2 local variables and then create Result at the very end.

Done.

https://codereview.appspot.com/83240043/diff/40001/physics/eg_spring_particle...
physics/eg_spring_particle.e:100: create Result.make (0.0, 0.0)
I don't understand the question...
The change is just esthetic...

https://codereview.appspot.com/83240043/diff/40001/physics/eg_vector2d.e
File physics/eg_vector2d.e (right):

https://codereview.appspot.com/83240043/diff/40001/physics/eg_vector2d.e#newc...
physics/eg_vector2d.e:47: "Create a new instance with `make' creation procedure
since EG_VECTOR2D is immutable. [03-2014]"
SInce EG_VECTOR2D has only one side-effect routine and that immutability for
simple and mathematical types is good

I think the immutability for simple and mathematical types is good.
Since EG_VECTOR2D has only one side-effect routine, I think the removal of it is
an acceptable pain relative to the benefit brought.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure.e
File view/eg_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure.e#newcode14
view/eg_figure.e:14: new_filled_list
On 2014/04/15 18:25:10, Manus wrote:
> I don't think there is a need to export it to NONE since it is already not
> exported.

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure.e#newcode96
view/eg_figure.e:96: model.set_name (l_attribute.value)
This patch change the semantic of {EG_ITEM}.set_name (see eg_item.e).

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_factory.e
File view/eg_figure_factory.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_factory.e#n...
view/eg_figure_factory.e:55: -- Element type for compilation purpose.
On 2014/04/15 18:25:10, Manus wrote:
> Add precondition.

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e
File view/eg_figure_world.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:35: make_with_model
Yes, I noted this dependency...

However `duplicate', `new_filled' and `make_filled' are not staticly void-safe
since `make_filled' use {G}.default.
Similarly to ARRAY.make, `new_filled' and `make_filled' should be deprecated and
`duplicate' refactored, no?

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:87: a_factory.set_world (Current)
On 2014/04/15 18:25:10, Manus wrote:
> Probably we should make `set_factory' obsolete?

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:158: "Use `has_linkable_figure' instead; [04-2014]"
On 2014/04/15 18:25:10, Manus wrote:
> This sounds wrong to me since it is looking for a EG_CLUSTER_FIGURE, unless
> there is somewhere a contract that says if we have a EG_CLUSTER then it can
only
> be a model of EG_CLUSTER_FIGURE, no?

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:435: not_has_a_link: not has_link_figure (a_link)
Indeed the invariant `consistent_link' of EG_GRAPH implies that if a link exists
in the model then the source of the link and the target of the link exists.

In first I wished replace the `if' with a `check' since `has_source_figure' and
`has_target_figure' ensure that the associated figures exist.
But since that we should avoid `check', I simply remove the unecessary
preconditions.

It is still the same problem:
Please, let me understand your viewpoint on my last comment
(http://www.eiffelroom.org/article/how_to_convert_your_code_to_be_voidsafe)

https://codereview.appspot.com/83240043/diff/40001/view/eg_figure_world.e#new...
view/eg_figure_world.e:1060: end
On 2014/04/15 18:25:10, Manus wrote:
> Should we just add a contract stating that both linkables_to_figures and
> links_to_figures are unchanged?

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_force_directed_lay...
File view/eg_force_directed_layout.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_force_directed_lay...
view/eg_force_directed_layout.e:261: l_linkables.new_cursor as it
Indeed,
but I have an issue with it:
Whan I write only `l_linkables' I get two compiler errors:
VOIT: Iteration expression should conform to ITERABLE [G]
VEEN: Unknown identifier `it'

However when I write `l_linkables.new_cursor' the compiler is happy...

I think it is a bug (I tried to open again ES and compile from scratch several
time).

Note: I currently use ES 14.05.9.4570

https://codereview.appspot.com/83240043/diff/40001/view/eg_link_figure.e
File view/eg_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_link_figure.e#newc...
view/eg_link_figure.e:73: if attached l_model.source.link_name as l_link_name
then
See your comment in eg_simple_factory.

https://codereview.appspot.com/83240043/diff/40001/view/eg_polyline_link_figu...
File view/eg_polyline_link_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_polyline_link_figu...
view/eg_polyline_link_figure.e:120: line_width: INTEGER
On 2014/04/15 18:25:10, Manus wrote:
> Why not adding a comment to all those queries?

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_resizable_cluster_...
File view/eg_resizable_cluster_figure.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_resizable_cluster_...
view/eg_resizable_cluster_figure.e:77: left: INTEGER
On 2014/04/15 18:25:10, Manus wrote:
> Add comment.

Done.

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_factory.e
File view/eg_simple_factory.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_simple_factory.e#n...
view/eg_simple_factory.e:45: attached node.attribute_by_name ("SOURCE") as
l_attribute_by_name and then
On 2014/04/15 18:25:10, Manus wrote:
> I see that now you check for both SOURCE and TARGET to be attached which is
good
> for a badly formatted XML case. Ignore my comment about the storing part.

Done.

[Conaclos, 2014-04-19 18:19:43]

https://codereview.appspot.com/83240043/diff/40001/view/eg_force_directed_lay...
File view/eg_force_directed_layout.e (right):

https://codereview.appspot.com/83240043/diff/40001/view/eg_force_directed_lay...
view/eg_force_directed_layout.e:261: l_linkables.new_cursor as it
After several test I find out why the bug occurs.

In fact it is an anchor type bug:

If I changes it:
l_linkables: like linkables

with it:
l_linkables: ARRAYED_LIST [EG_LINKABLE_FIGURE]

it compile without `new_cursor'.