X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f X-Recipient: geda-user AT delorie DOT com Date: Sun, 7 Feb 2021 17:05:07 +0100 (CET) From: Roland Lutz To: "Glenn (glimrick AT epilitimus DOT com) [via geda-user AT delorie DOT com]" Subject: Re: [geda-user] recursive subsheet issue In-Reply-To: Message-ID: References: <0cc19ac4-217d-0897-52db-8b6fdcbbd975 AT epilitimus DOT com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1764072999-1612713907=:2567" 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-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 >> 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 >> 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 >> 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--