Re: gEDA-user: gsch2pcb use pcb library preferences patch

[Peter Clifton <pcjc2@xxxxxxxxx>]

On Tue, 2009-03-10 at 17:47 -0700, Josh Jordan wrote:
> This patch lets gsch2pcb open up ~/.pcb/preferences and read the
>    library-newlib = paths and add them to the list of places to get
>    footprints.  The patch is directly for gsch2pcb.c

If you could send patches readdy for applying, that would be great. I'm
noting that we don't actually want:

+       printf("going into new code\n");
+       printf("coming out of new code\n");

or any other of the live debugging printf patched into gsch2pcb.


I think the general idea of the patch is fine. We have various plans to
completely re-write how gsch2pcb works at some point (making it defer
operations to PCB), but in the absence of solid progress on that front,
I don't see how improving the exsting gsch2pcb hurts anything.



+                       if (g_str_has_prefix(buf, "library-newlib = "))
+                       {
+                               pref_paths = buf+17;

Code like this is very dangerous from a maintenance point of view.
You could use:

pref_paths = buf + strlen ("library-newlib = ");

(Bonus points for storing that string in a variable or #define). Smart
compilers "ought" to be able to optimise out the strlen call of a static
string, and if not.. we're not exactly on a time-critical path here.

+                                       while(*++tmp != ':' && *tmp != 0); //push np up to next : or end of string

This breaks portability to Win32, where we use ";" as a path separator.
I presume you want to change the "np" in the comment to "tmp"?

You'll notice that the last context after your diff is:

 #define PCB_PATH_DELIMETER ":"

This suggests that we already had a portability problem there. In any
case, you should put your changes after that define, and use it rather
than hard-coding the delimiter.

I doubt any of our existing code gracefully handles ":" or ";" in
filenames, so I'm not going to gripe that yours doesn't allow any
escaping of those characters to treat them as part of the file-names.
Given that you're not handling escaping (which might require you to walk
the string manually), g_strsplit() might be a neater way to dice up the
path. (It would be more easy to visually inspect for correctness than
traditional pointer arithmetic based string walking.) 

I note that you're not using similar code to that existing in gsch2pcb
to parse the path string (just after your changes). That code uses
strtok, however the man page for that routine suggests it should be
avoided for various reasons. (IE.. don't use it in your new code).

A patch to make both code blocks use the same method (whatever that is)
would be welcome. I'll also note that they could probably be refactored
to share that common code (once you've stripped off the "library-newlib
= " prefix (using pointer arithmetic).


Stylistic points.. Your code's indenting doesn't seem to match the
existing gsch2pcb code (or the typical convention used in the rest of
the main gEDA code-base). Additionally, the gEDA code base does not
generally use "//" comments. Instead, use /* ..... */ blocks.


I hope these comments are useful, and help you improve the patches
further. it is always nice to have new people working on gEDA / PCB, and
this is certainly an area which hasn't seen a lot of active maintenance
for a while now.

Best regards,

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)



_______________________________________________
geda-user mailing list
geda-user@xxxxxxxxxxxxxx
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user