[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: gEDA-user: gsch2pcb use pcb library preferences patch
Thanks for the comments. I have made suggested changes. I have tried
using g_strsplit and found it to takes more lines of code and uses
more memory. I prefer the changes to be walking-style instead of
g_strsplit due to fewer lines, less allocated memory, and it will be
easier to make it accept escape characters or other tweaks. I made a
function to add libraries that replaces the strtok code and can be
used if more libraries need to be added.
-Josh Jordan
--- On Wed, 3/11/09, Peter Clifton <pcjc2@xxxxxxxxx> wrote:
From: Peter Clifton <pcjc2@xxxxxxxxx>
Subject: Re: gEDA-user: gsch2pcb use pcb library preferences patch
To: "gEDA user mailing list" <geda-user@xxxxxxxxxxxxxx>
Date: Wednesday, March 11, 2009, 12:38 PM
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
[1]geda-user@xxxxxxxxxxxxxx
[2]http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
References
1. file://localhost/mc/compose?to=geda-user@xxxxxxxxxxxxxx
2. http://www.seul.org/cgi-bin/mailman/listinfo/geda-user
--- gsch2pcb_orig.c 2009-03-13 00:37:48.000000000 -0400
+++ gsch2pcb.c 2009-03-13 00:36:39.000000000 -0400
@@ -1229,6 +1229,32 @@
return 1;
}
+#define PCB_PATH_DELIMETER ":"
+static void
+add_library_paths(gchar *paths)
+ {
+ gchar *tmp, *path, *epath;
+ tmp = g_strchomp(paths);
+ while(*tmp)
+ {
+ paths = tmp;
+ while(*tmp++ != PCB_PATH_DELIMETER[0] && *tmp);
+ if(*(tmp-1) == PCB_PATH_DELIMETER[0])
+ path = g_strndup(paths, strlen(paths)-strlen(tmp)-1);
+ else
+ path = g_strndup(paths, strlen(paths)-strlen(tmp));
+ epath = expand_dir(path);
+ g_free(path);
+ if (g_file_test(epath, G_FILE_TEST_IS_DIR))
+ {
+ element_directory_list = g_list_append(element_directory_list, g_strdup(epath));
+ if (verbose)
+ printf("added library path- %s\n", epath);
+ }
+ g_free(epath);
+ }
+ }
+
static void
load_project(gchar *path)
{
@@ -1468,22 +1494,32 @@
element_directory_list = g_list_append(element_directory_list,
"packages");
-#define PCB_PATH_DELIMETER ":"
- if (verbose)
- printf ("Processing PCBLIBPATH=\"%s\"\n", PCBLIBPATH);
-
- path = g_strdup (PCBLIBPATH);
- for (p = strtok (path, PCB_PATH_DELIMETER); p && *p;
- p = strtok (NULL, PCB_PATH_DELIMETER))
+ #define GTK_LIB_PREF_KEY "library-newlib = "
+ /* load library paths from pcb preferences ~/.pcb/preferences */
+ path = expand_dir("~/.pcb/preferences");
+ if (g_file_test(path, G_FILE_TEST_IS_REGULAR))
{
- if (g_file_test(p, G_FILE_TEST_IS_DIR))
- {
- if (verbose)
- printf ("Adding %s to the newlib search path\n", p);
- element_directory_list = g_list_append(element_directory_list,
- g_strdup(p));
+ char buf[1024];
+ gchar *pref_paths;
+ FILE *fp;
+ fp=fopen(path, "r");
+ while(fgets(buf, sizeof(buf), fp))
+ {
+ if (g_str_has_prefix(buf, GTK_LIB_PREF_KEY))
+ {
+ pref_paths = g_strchomp(buf + strlen(GTK_LIB_PREF_KEY));
+ if (verbose)
+ printf ("Processing %s\"%s\"\n", GTK_LIB_PREF_KEY, pref_paths);
+ add_library_paths(pref_paths);
+ }
}
+ fclose(fp);
}
+
+ if (verbose)
+ printf ("Processing PCBLIBPATH=\"%s\"\n", PCBLIBPATH);
+ path = g_strdup (PCBLIBPATH);
+ add_library_paths(path);
g_free (path);
pins_file_name = g_strconcat(basename, ".cmd", NULL);
_______________________________________________
geda-user mailing list
geda-user@xxxxxxxxxxxxxx
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user