On Aug 5, 2008, at 5:00 AM, Peter TB Brett wrote:
> Hi Ed,
>
> I got time to look through this patch, in the end.
>
> Code external to the library doesn't really need to know about how the
> o_shortest_distance() function is implemented. I strongly recommend
> putting
> all the declarations of the of the o_<type>_shortest_distance()
> functions
> into libgeda/include/prototype_priv.h instead of prototype.h, so
> that only
> libgeda code can use them directly. Don't forget that it's much
> harder to
> remove a function from a library's public interface than to add one!
Good, I can move the prototypes. I now also think the functions
should not take OBJECT, but their individual type:
o_circle_shortest_distance(CIRCLE* circle, int x, int y);
I don't think these functions need to share the same signature anymore.
> Also, a few other points.
>
> Firstly, is it possible to make your editor highlight trailing
> whitespace?
> In my Emacs 22 .emacs, I have:
>
> ;; Make file buffers use tab-completion and highlight trailing
> whitespace
> (add-hook 'find-file-hook
> (function (lambda ()
> (make-local-variable 'show-trailing-whitespace)
> (setq show-trailing-whitespace t))))
Done.
> Secondly, you're using g_assert() quite a lot. In particular, you
> assert
> that the object isn't NULL in o_shortest_distance(), but you then do
> exactly the same assertion in each of the type-specific functions!
Yes. I like assertions. (Well, in C at least.)
> A few months back, I thought carefully about this and wrote a
> detailed blog
> post about my conclusions. In particular, we assume that libgeda
> works --
> that is, libgeda functions always pass valid data to other libgeda
> functions. We also try to avoid killing the application if we
> possibly can.
>
> http://www.peter-b.co.uk/blog/2007/11/error-reporting-handling-in-libgeda.html
I don't agree with the portion on assertions, but if it's the
philosophy of the project, I don't mind removing them.
Asserts stop the program closest to the problem. Then, by looking at
the callstack (and possibly the stack frames), the developer can
isolate the problem quickly. If the program continues operation and
fails later, the developer has a more difficult time tracing down the
problem. The function may have already gone out of scope and it's
stack frame would not be available. The developer may not even know
all the preconditions and postconditions for all the functions that
executed, consuming more of the developer's time to analyze the code.
In this case, assertions not only document the pre/postconditions, but
also automate testing of the pre/postconditions during execution.
(Beats printf's by a mile.)
If you are set on not having asserts in release code, I would leave
asserts in for developers, and then compile the release code with the -
DG_DISABLE_ASSERT option or define it in an include file. I've seen
some DEBUG macros, so maybe even:
#ifndef DEBUG
#define G_DISABLE_ASSERT
#endif
could be placed before the includes. Anyway, I've had favorable
experience with assertions, so I like them.
> Instead of causing an assertion failure when the input isn't valid,
> consider issuing a g_critical() warning and then returning your
> 'couldn't
> be calculated' value. Have you considered using a negative return
> value to
> signify an error, instead of the maximum double?
I consider returning DBL_MAX from o_complex_shortest_distance() and
o_text_shortest_distance() to be normal operation and not an error
condition. But I consider the invalid case in o_shortest_distance()
to be error condition. If an error needs to be reported, I'd probably
return a separate boolean value and not mix it up with the result.
But, returning an error condition requires the callers (who care) make
a special case to test for the condition. Just returning a value for
this case is benign. This error condition shows the classic example
of garbage-in garbage-out. For the error condition, the value could
just as well be 42. However, with a little white-box knowledge of the
caller, returning DBL_MAX causes the caller to skip the invalid
object. In retrospect, by my own coding style, I probably should have
placed an assertion here. :)
> Finally, instead of using DBL_MAX, please use G_MAXDOUBLE.
Ok. Consequently, by G_MAXDOUBLE's definition, I should probably
change double to gdouble.
Cheers,
Ed
_______________________________________________
geda-dev mailing list
[email protected]
http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev
