X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient: geda-user AT delorie DOT com
Date: Wed, 2 Dec 2020 13:40:43 +0100 (CET)
From: Roland Lutz <rlutz AT hedmen DOT org>
To: "Glenn (glimrick AT epilitimus DOT com) [via geda-user AT delorie DOT com]" <geda-user AT delorie DOT com>
Subject: Re: [geda-user] SAB processing patches
In-Reply-To: <1b2c64b3-6a36-c1f3-dd54-bb583c6bea17@epilitimus.com>
Message-ID: <alpine.DEB.2.21.2012021253440.1262@nimbus>
References: <e4ff3c96-939b-a93e-a32f-5e938b6daa63 AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2011302045040 DOT 2894 AT nimbus> <20201130220505 DOT 0AE4282C54FD AT turkos DOT aspodata DOT se> <7c75ed03-456c-b408-8b50-0448f6b3a04f AT epilitimus DOT com> <alpine DOT DEB DOT 2 DOT 21 DOT 2012011606400 DOT 6785 AT nimbus>
 <1b2c64b3-6a36-c1f3-dd54-bb583c6bea17 AT epilitimus DOT com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-804920301-1606912843=:1262"
Reply-To: geda-user AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: geda-user AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-804920301-1606912843=:1262
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT

Hi Glenn,

On Tue, 1 Dec 2020, Glenn (glimrick AT epilitimus DOT com) [via 
geda-user AT delorie DOT com] wrote:
> 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?

this doesn't work out-of-the-box because you sent patches, and I 
re-applied them to Git, changing the commit ID.

Ideally, you would push patches to a publicly visible Git repository 
(self-hosted or Github, it doesn't really matter) and send a pull request. 
Not the thing Github calls a pull request, but a mail containing something 
along the lines of "please pull commit <full commit ID> from branch 
<branch> at repository <url>" along with a summary of your changes.  You 
can generate this in a standardized form with "git request-pull".

Now we have the situation that I have a branch which contains the same 
thing as, but is not ID-entical to, your branch, so you can't easily pull 
my changes.  Instead, just cherry-pick the latest commit:

     git checkout sab
     git cherry-pick 2de3373

>> - 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?

I did a quick grep: there are a few places which use lower() in gnetlist, 
but they are either handling filename extensions (which you'd want 
gnetlist to recognize regardless of capitalization), or they are in 
backends (which should do whatever is appropriate for the given tool).

> 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.

Yes, that's somewhat unfortunate.  I believe the "correct" way to solve 
this is by separating the gnetlist "filter stages" (i.e., slotting, 
hierarchy, packages) more cleanly and allowing the user to specify a 
"filter pipeline" on the command line.  This would allow handling refdes' 
much cleaner in SPICE backends by disabling the "packaging" stage, and at 
the same time allow adding a custom "SAB" stage.

This has been on my mind for years, but so far, there have always been 
more pressing issues.

>> - 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.

Please do fix any issues you encounter.  Chances are nobody else will 
touch that particular piece of code for some time, so if you don't fix it, 
it won't be fixed.

> 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.

You can actually specify a custom comparison function with sort/sorted 
which could e.g. compare "None" higher than any numerical value.

>> - 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.

This is the more "Python-ic" way to do it.  For example, the preferred way 
to handle elements which are missing from a dictionary in Python is:

     try:
         val = d[key]
     except KeyError:
         ...handle missing value...

instead of the more C-like

     val = d.get(key, None)
     if val == None:
         ...handle missing value...

It took me some getting used to, as well, but in the end, I believe it 
makes for more readable code.

Roland

--8323329-804920301-1606912843=:1262--