Comment 8 for bug 996319

Hi guys,

As you might have noticed, I'm currently somewhat in the process of breaking connectivity scanning code, and our existing find.c implementation is a mess.

This might impact upon this patch slightly, so we should adopt one of two strategies..

1. Assume impact will be minimal, and fix-up the patch later.
2. Merge it to core ASAP, and make it my problem not to break it ;)

I was wondering whether you thought it might be appropriate to refactor out some of the netlist extraction code from this branch into the core. (Where it can be cleaned up along withe the existing stuff).

A few style points...

1. Please send unified diffs, as they are easier to read.
2. Please don't add comments with your name or initials into the code, e.g. //JM // END JM etc... use the git history to track these things.
3. Not a requirement, but I usually mark a file's copyright as
"Copyright (C) $YEAR PCB Contributors (see ChangeLog for details)"

If you wrote the whole file, feel free to add yours as well, but I always like to leave the "PCB Contributors" as a second line, as this encourages people to feel contribution is welcome, and not the sole preserve of the original author / copyright holder.

4. I want to see changes to the core (like the addition of LookupConnectionByPin() committed as a separate commit.
5. PLEASE don't re-introduce type aliases with "Ptr" on the end. These are gone now in PCB, good riddance ;)

IPCD356_AliasPtr and IPCD356_AliasListPtr should become IPCD356_Alias * and IPCD356_AliasListPtr *

6. Looking at the above structures, we have a few problems:

+ char NName[11];
+ char NetName[256];

I believe PCB supports arbitrary length in its net names (and if it doesn't, it should!) - so I'm unhappy to see this fixed length field. I appreciate the limit of 14 characters on the IPC standard, so that is OK,

I'm not sure why you limit the aliased net names to 10 characters (+termination), I thought the standard supported up to 14? Is this due to the format of your alias strings?

7. PCB's core now depends on glib, so feel free to use GLists, hash tables, string functions, whatever makes the code easier. I think IPCD356_AliasList is a candidate for being replaced if you wanted.

8. Lots more [256] limited strings, and string handling without bounds checking in the code. I'm really trying to keep people from introducing code like this on both grounds of: 1. Arbitrary length limitations are bad, and 2. Unsafe string handling which could cause a crash is bad.

9. Refactoring is better than copy+paste alteration... e.g. rather than adding ResetVisitPinsViasAndPads() you could have extended ResetFoundPinsViasAndPads().

As it happens, I ran into some buggy code which needed refactoring recently, and did just that... so your calls now become:

- ResetFoundLinesAndPolygons (false);
- ResetFoundPinsViasAndPads (false);
+ ResetConnections (false, FOUNDFLAG);

(ResetConnections combines a call to ResetFoundLinesAndPolygons and ResetFoundPinsViasAndPads)

And I think (not build-tested) your new function ResetVisitPinsViasAndPads can be removed if you change:

- ResetVisitPinsViasAndPads ();
+ ResetFoundPinsViasAndPads (false, VISITFLAG);

Yes.... reading the names back to myself, I realise the next step of my refactor should have been (might yet be) renaming the above functions to avoid the word "Found" or "Connections", as the functions are now more generic than that. (And mixing the terms Found / Connections will be even more confusing once I introduce the "CONNECTIONFLAG" shortly.