kicad-developers team mailing list archive

[Lorenzo Marcantonio <l.marcantonio@xxxxxxxxxxxx>]

OK, I think I've got bug-for-bug compatibility with trunk in my branch,
the split of the GAL layer number seems to work fine now.

I know that it's all in early phase and work in progress, so I feel
'normal' that many things are not working (like color changing, the zoom
function - and why meta-F1/F2 and not F1/F2 like before, can't hide some
things and show on). 

However since nobody ever mentioned yet there seems to be an off-by-one
error or a polygon misclosing in the zone edge drawing: when there is an
'hole' in a zone instead of closing it and lifting the pen, jumps
drawing to another area (with the pen down:P). 

I am not very fond of the approach of the PCB_PAINTER, which contains
and switch-dispatch drawing of entities... why not delegate as usual to
the single BOARD_ITEMs? (or maybe define a PCB_PAINTABLE interface, or
something)

Also, more like of a curiosity, why use deques in the stroke font code?
I'm a little rusty with STL but vectors seems a more likely fit... given
the usage pattern I feel there's something with the performance of
vector::push_back (IIRC deques allocate in blocks, I don't remember the
details). Also for some reason text drawn with opengl is less 'crisp'...
maybe it's the coordinate rounding, I don't know.

At last, the details on the type split: it works exactly at expected.
The 'trick' is as before in layers_id_colors_and_visibility.h, attached
as a preview for the curious ones, full code in my branch. It still uses
the enum implementation so people which didn't like it a couple years
ago still will not like this one :D

GAL_LAYER_NUM has in fact the same numeric values of the int previously
in use, however is better protected since it's not an int and it's type
checked. Macros ITEM_GAL_LAYER and PCB_GAL_LAYER are used for moving
from the PCB_VISIBLE/LAYER_NUM domain to GAL_LAYER_NUM. Sadly these are
macros (and ugly ones) because these need to be used in const exprs, so
overloaded operators or functions can't be employed... *maybe* the new
constexpr keyword could help, I don't know exactly how it works. I'm
pretty sure there is no performance penalty since all of these
conversion can be actually compiler folded. For experience I know g++ is
sufficiently smart.

For the reverse thing: at the moment getting back the LAYER_NUM from a
GAL_LAYER_NUM is in ROUTER_TOOL, members pickSingleItem and
updateStartItem. I hand coded a substitution inline, probably the best
way to handle it is to use a conversion function with a behaviour like
dynamic_cast<>, returning UNDEFINED_LAYER if the conversion can't be
done (or maybe trigger an exception/assert?).

Another nuisance: even if LAYER_NUM IS-A GAL_LAYER_NUM (in the Liskov
sense, too), enum are deemed distinct and can't inherit, so there is
a need for overload utility functions (look for IsCopperLayer); as usual
a class and the new class literals would help, but I don't know how much
they are stable/implemented (look here
http://www.preney.ca/paul/archives/636).  Also they are only in gcc 4.7,
I think, so it's not a lightweight decision to take.

Of course this is only if you like the type safe approach; if you want
it I can backpatch to trunk; if you just want the type tag as a typedef
for better code documentation only (like it was done for
LAYER_NUM/LAYER_MSK) I can prepare a patch for this too. Otherwise just
don't care about it:P:P

-- 
Lorenzo Marcantonio
Logos Srl

Attachment: layers_id_colors_and_visibility.h.gz
Description: application/gunzip