On Dec 18, 2008, at 7:16 AM, Peter Clifton wrote:
>
> Each file we compile takes a certain overhead of time. For
> collecting a
> few simple routines such as m_*_shortest_distance(), I'd prefer to see
> one new file, m_gemetry.c rather than several small files. Having lots
> of very small files also makes navigating and editing the source
> harder
> in some cases.
I can see m_box.c and m_circle.c gaining functions over time. If so,
keeping them in separate files increases each module's cohesion.
Keeping the function name's prefix the same as the base filename seems
to follow a convention in the code. Also, having the function name's
prefix associated with the first parameter ("this") also seems to
follow a convention in the code. These two conventions have made
navigating the code relatively easy for me.
I'm not sure about the compiler overhead for each file -- everyone's
mileage may vary. For an incremental build, make should not launch
the compiler if the object file is up to date. Maybe some engineers
do full builds all the time? I haven't seen many engineers make this
aspect a priority. (Except for include sentinels, maybe.)
> gint, gdouble vs. int and double:
>
> Please can we use the basic types int and double etc.. where possible.
> This gives shorter lines, more standard C code (if you ever wanted to
> port away from depending on glib), and syntax highlighting in some
> editors works better if you use the standard ones. gEDA also has the
> convention of using int rather than gboolean in many places.
I don't mind which way this goes, but so far, the changes I've been
asked to make to my patches are inconsistent. I've been asked to use
glib numeric constants instead of C constants, then use C fundamental
types instead if glib types.
Is eliminating libgeda's dependency on glib really a requirement?
> + solid = object->fill_type != FILLING_HOLLOW; /* FIXME */
>
> We should avoid adding "FIXME" comments without any indication of what
> might need to be fixed. (First of all, avoid adding things which need
> fixing ;)). I'm not sure what the issue is here, so I doubt anyone
> looking back at the code in the future will either.
The line of code does not fit within the responsibility of the
function. It should be changed to a macro or function.
#define IS_OBJECT_SOLID(object) ((object)->fill_type != FILLING_HOLLOW)
I didn't put it anywhere yet, because I'm uncertain of naming
convention for the new macro or function.
> What about altering the prototype of the o_shortest_distance()
> routine:
>
> +double shortest_distance (OBJECT *object, int x, int y, int
> override_solid)
This would eliminate the other switch statement. A definite
improvement.
Also, I would create a wrapper function so the override_solid
parameter is not exposed in the public interface. The parameter
causes specialized behavior required internally by libgeda, so hiding
it from the public interface seems appropriate.
> Passing that override solid as a parameter to all the object
> specific functions:
>
> (NB: As you'll see from my mods, keeping the function signature for
> all
> of these identical makes for a shorter dispatch function, so if we did
> this, we should pass the override for all object types).
I don't understand the reasoning behind using function pointers inside
the switch statements.
(Off the subject: I've created switch statements where cases were in
numerical order with no gaps, and the GNU compiler optimized it to a
jump table. I was amazed. I've never seen that with any other
compiler.)
> Doing it this way, you wouldn't necessarily need to split out an
> m_geometry.c for shortest-distance functions which take BOX * and
> CIRCLE
> * parameters.
The functions in m_box.c and m_circle.c are more generic. The
proposed function prototype in o_basic.c would be specialized for
object selection. If libgeda gets used for more applications in the
future, it is probably better to keep the functions on the generic side.
Cheers,
Ed
_______________________________________________
geda-dev mailing list
[email protected]
http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev
