Mail Archives: geda-user/2021/02/07/11:25:31
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-1764072999-1612713907=:2567
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT
Hi Glenn,
thank you for your contribution! :-)
On Sat, 6 Feb 2021, Glenn (glimrick AT epilitimus DOT com) [via
geda-user AT delorie DOT com] wrote:
> Assuming I remembered correctly how to do it you should now find 3 new
> commits on the stable-1.10 branch in my repo.
`stable-1.10' is only for fixes which should go into the next bugfix
release; regular development usually happens on `master'.
>> commit 32d8fb59f919c5a26b5dae90a439f45c2ed7f86f
>> Author: Glenn Pavlovic <glimrick AT epilitimus DOT com>
>> Date:Â Â Sat Feb 6 21:24:53 2021 -0800
>>
>> Â Â Â Block inifinite subsheet recursion
A few things which I noticed when looking at the patch:
1) The recursion check compares the os.path.realpath of the schematic,
which is not necessarily a good idea. As a rule of thumb, if you don't
have to mess with symbolic links in your code, don't. (I know gEDA/gaf
violates this rule on a few instances, but this is because I wasn't sure
if I'd break anything by changing them, and shouldn't be used as an
example for new code.)
2) Using the filename for comparison isn't necessary anyway since the
composite_sources field already contains Schematic objects; you can simply
compare these objects.
3) The current comparison is done at the top of s_traverse_sheet1, but I
think it makes more sense to do the comparison in the loop which invokes
s_traverse_sheet1. This means top-level schematics aren't checked, but
these can't be recursive anyway.
4) Passing a common `stack' variable as an argument to s_traverse_sheet1 is
kind of mixing paradigms:
a) Either `stack' is conceptually shared between s_traverse_sheet1
invocations. In this case, there's no need to pass it as an argument;
s_traverse_sheet1 can just use the `sheet_stack' variable. (I'd prefer to
place the definition before the function body to make things clearer, but
this isn't strictly necessary.)
b) Or, s_traverse_sheet1 passes the current invocation stack to the next
level of hierarchy. In this case, it's conceptually cleaner to pass a
copy of the variable which is discarded afterwards. Since you don't need
to do stack.pop() in this case, the `set' type seems more appropriate:
s_traverse_sheet1(sheet, set())
...
s_traverse_sheet1(subsheet, set.union(stack, {sheet_path}))
5) A change like this should have a regression test, i.e., a minimal test
in xorn/tests/netlist/regression/ which fails before applying the change
and passes afterwards. This way, if someone inadvertently re-introduces
the bug, the test will fail and the error be noticed.
Minimal means that the test should only contain the bare minimum of what's
necessary in order to trigger the bug, which usually means hand-optimizing
the test in a text editor and using (usually just 1–2) custom symbols.
In this case, you can probably trigger the bug using only one schematic
and one symbol.
6) Keep Python coding style in mind:
a) operators are preceded and followed by a space, and commas are
followed by a space
b) `if' is followed by a space and doesn't have parentheses around its
condition
c) iterating over a sequence is preferentially done using the `for
member in sequence' syntax, on in this case, `for member in
reversed(sequence)'
d) an `if' statement which handles an error condition and ends with a
`return' statement or similar should usually have its condition be true on
error and not have an `else' clause
7) Netlist errors should be concise and not contain line breaks. In this
case, the important information is the name of the instantiating schematic
(which is implicitly reported by Sheet.error) and the name of the
instantiated schematic; only the latter should be included in the error
message.
>> commit 23b15ba475ec26fb55727af2903f17fae371c4e5
>> Author: Glenn Pavlovic <glimrick AT epilitimus DOT com>
>> Date:Â Â Sat Feb 6 21:55:22 2021 -0800
>>
>> Â Â Â Fixed a rare, freaky bug
Do I understand correctly that this bug can only be triggered by a
recursing subsheet, which is caught by the previous commit?
In this case, I'd prefer to not handle this case specially. An invalid
internal state can lead to errors, but it's the invalid internal state
which should be fixed, not the assumption that the state is sane.
>> commit 1f2de17cce2e4e97b8be14dda145155d5d8bd1ae (HEAD -> stable-1.10)
>> Author: Glenn Pavlovic <glimrick AT epilitimus DOT com>
>> Date:Â Â Sat Feb 6 22:05:27 2021 -0800
>>
>> Â Â Â Remove a cruft comment
This comment was a result of me erring very hard on the side of not
removing comments. I believe it is safe to remove it.
Roland
--8323329-1764072999-1612713907=:2567--
- Raw text -