Mail Archives: geda-user/2020/12/01/21:47:36
Roland,
Roland Lutz wrote:
> I have fixed the remaining formatting issues as I encountered them:
>
> Â Â Â https://github.com/rlutz/geda-gaf/commits/glenn-sab
So I'm stumped (or maybe just dense, the way this day has gone...) My
repository was pulled from git.geda-project.org and I named my branch
just sab. So how do I pull your changes into my branch so I don't send
you a patch that undoes all your fixes?
>
> So here's the next batch of feedback. I still haven't got to the core
> of it (like "does this make sense" and "is this implemented in a way
> that's copasetic with the rest of gEDA/gaf"), but I'll get there
> eventually.
yes and yes?
>
> - Most things in gEDA/gaf are case-sensitive, so it makes sense to treat
> Â SAB arguments as case-sensitive, as well.
>
Hmm...okay. so no str.lower in gnetlist and netlist.py?
> - getlist isn't designed for backends to change component/package names,
>  much less blueprint names. This may work, or it may not. If you need
> Â to rename things, it's better to use a filtered component/package list
> Â instead (see the SPICE backends for an example).
>
Not entirely sure I understand this one. Some general points:
1. I think maybe I am looking at gnetlist in a more general context of
"a tool to work with gaf.netlist.netlist" rather than just "a tool to
generate netlists" if that makes sense.
2. The backend isn't changing anything. A script executed by SAB can but
that happens before the backend is called.
Now assuming we are talking about modify_refdes:
3. modify_refdes is not intended to be a part of the "official" code
base, but rather an example script which is why it is in the examples
directory rather than the mainline code.
4. My default position is the user is always right so if the user
decides to run a script which changes their refdes then so be it, it's
not for me to tell them they shouldn't.
5. At least for the specific case of spice-sdb (and by extension the new
xspice backend I am working on, which is derived from spice-sdb) you
have to change the blueprint refdes to have any effect because that is
what spice-sdb uses for the refdes and ignores the higher levels.
> - You don't need "nobase_" on your "example_DATA" target (you don't have
> Â files in subdirectories), but it doesn't hurt, either.
>
okay, noted. I wasn't familiar with how automake handled python so I
just copied a Makefile.am from another directory and hacked. It worked
so I just left it and moved on.
> - Fixes to existing code (like the memory leak in s_slot.c) should go
> Â into a separate patch.
>
Noted
Valgrind shows several others, actually it shows lots but most are in
linked libraries not in geda/gaf. I only fixed this one because I was
right there working.
>
> - Header files (especially public ones) should only include those other
>  header files which they minimally require in order to work. AFAICT,
> Â for "sab.h", this would be just "struct.h".
>
Agreed. I'll need to look to be sure but the extras may well be left
over cruft. I know there was a reason I put them there I just don't
recall if that reason is still valid.
> - Don't assign variables inside function calls unless you have a good
>  reason to do so. Assigning from "strtok" inside "while" in sab.c is
> one
> Â of the few cases where this is ok (as the assignment can't be put on a
> Â separate line without hurting readability); most other cases are not.
>
<sigh> I always think I have a good reason! B)
> - "TOPLEVEL *" variables are called "toplevel" (all lower-case) in the
> Â existing code, so it's a good idea to follow this convention for new
> Â code, as well.
>
on the todo list.
>
> - gEDA/gaf Python code should reside within the "gaf" top-level package.
> Â You basically have two choices for organizing the SAB code: either
> Â (1) have individual sub-modules of gaf by placing the code files in
> Â Â Â Â Â "xorn/src/python/", then import "gaf.sab" and call
> Â Â Â Â Â "gaf.sab.process", or
> Â (2) group the SAB source as a sub-package by placing the code in
> Â Â Â Â Â "xorn/src/python/sab/", then import "gaf.sab.sab" and call
> Â Â Â Â Â "gaf.sab.sab.process"
> Â I'd go with the first option, but either way is okay.
>
okay, I'll move it, #1 looks cleaner to me.
> - Don't re-define "process" in "sab/__init__.py". There is some point
> Â in doing this for overloaded or publicly-visible functions, but here
> Â it just confuses things.
>
noted but moot once I move sab to gaf
> - You are processing the "--sab-context" command-line argument in
> Â "netlist.py", so it is not necessary to process it in "gnetlist.in",
>  as well. You can just pass it through unchanged.
>
yeah this is a bit of leftover. I actually wrote it in gnetlist first
and had a more restrictive syntax in netlist.py and then it occurred to
me that xorn netlist could be called independently and so I needed to do
it there as well. I'll look at it and make sure there's nothing else I'm
doing there and change it if not.
> - User-visible messages should be wrapped in _("...") in order to be
> Â translatable.
>
okay
> - Since you use the tuple (order, action, parms) in multiple places, it
> Â may be worth defining a collections.namedtuple for that purpose.
>
I'll look at it
> - Don't use something like 1000000000 as a special value. If you have
> Â to store the absence of a value in-band, use something that cannot
>  ever occur as a valid value, like -1 or None. (And if you *really*
> Â have to use a magic value somewhere, please define it as a constant.)
>
Here there was actually a reason to do it this way. By assigning a large
positive number as the *default* order number, when I sorted them all
the unordered actions end up at the end of the list naturally. But I can
certainly make it a constant. The alternative was to have two lists, the
ordered actions and the unordered ones which I really don't like.
> - In modify_refdes.py, the variable "refdes" is either a list or a
>  string. Since the expression "'C' in ..." works in both cases, I
> Â suggest replacing "list(levels.upper())" with "levels.upper()".
ummm....let me look at this in more detail and get back to you.
>
> - You don't need types.StringType and types.ListType;
> Â "isinstance(..., str)" and "isinstance(..., list)" work just fine.
>
okay, did not know this
> - Using "dir(m)" to determine whether a variable is defined in a module
>  feels a bit too hacky. I'm not sure yet how I'd prefer this to be
> Â solved; if you absolutely have to make the code depend on whether or
> Â not there is such a variable, use it and catch an AttributeError.
>
For me coming from a C/C++ background exceptions are supposed to be for
exceptional, i.e. unpredicatable and serious, cases not alternate cases.
So to me the natural path is to check and proceed accordingly rather
than assume and deal with the consequences if you are wrong. Using
exceptions here strikes me as hacky. Also from an execution perspective
it is more efficient to test rather than use exceptions since there is
no stack unwinding. At least at this point, most backends do not have
SAB_CONTEXT (one of the two cases I think you are referring to) so the
exception pathway would be the most likely to get tripped. But...
If having read all that you still aren't convinced, I will change it and
shudder every time I look at it. Looks like one of us is going to have
to live with the heeby-jeebies on this one. :)
> - Instead of:Â Â Â Â Â ''.zfill(4 * indent).replace('0', ' ')
> Â you can just do: 'Â Â Â ' * indent
>
okay, again did not know that.
> - Bonus points for using str.partition! I didn't know that one.
>
Now if I could just get my tractor started...
Glenn
- Raw text -