kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 04/28/2013 06:41 AM, Lorenzo Marcantonio wrote:
> I was comparing my tree with the master one. These are some changes
> I did in times and I would like what do you think of these
> 
> - GenDate() at the moment uses a fixed format (more or less the european
>   one), with the months hardcoded to the short english names.

GenDate() is not my function, so I could only offer an opinion but no protective mandate.
 I am neutral, and to formulate a strong opinion would require a study of its usage, for
which I do not have the time.


> 
>   Wouldn't it be better to use the localised formatter? I'm using 
>   "%e %B %Y" but even better would be the strftime %x formatter
> 
> - In the grid drawing code there is a special case to show
>   a double-pitch grid before suppressing the drawing. That's confusing
>   if you're using the grid to estimate distances. So if the grid doesn't
>   fit, just hide it


Mildly in favor of your argument.

> 
> - Many of the macros in macros.h would be better defined as templated
>   functions (unless there is some weird preference for macrofunctions in
>   C++). In particular EXCHG wouldn't need the boost typeof machinery.
>   It's pretty simple, actually:
> 
>   template<class T, class T2> inline void EXCHG( T& a, T2& b )
>   {
>     T temp = a;
>     a = b;
>     b = temp;
>   }
> 
>   A similar approach holds for the other ones. If it wasn't for some
>   mixed-type usage (double with int, for angles, IIRC), std::swap would
>   be even better.

std::swap seems OK, but it may cost us compile time by bringing in a header or two
everywhere.  Your template is also OK and perhaps leaner to compile.  Either is superior
to the macro.

> 
> - The same hold for SAFE_DELETE; the current one in particular is *not*
>   protected against accidental single-statement use i.e.
> 
>   if (stuff) SAFE_DELETE(x);
> 
>   generates the equivalent
> 
>   if (stuff) delete x;
>   x = NULL;

SAFE_DELETE is not my code.  But I do not like it in any fashion.

I would rather see it removed in total.

	SAFE_DELETE( p );

replaced with:
	delete p;
	D( p = 0 );   // if the null is needed at all

Reasoning: we can at least see what is happening.  SAFE_DELETE tells me absolutely
nothing, other than that my normal delete is not safe, and that is hogwash.

A C++ programmer should know that

	OBJ* p = NULL;
	delete p;

is harmless.


> 
>   which is clearly wrong; if (for some unknown reason) a macro is
>   preferred at least we should add a do { ... } while (0) guard
> 
> - ReturnLayerPair is a strange name for an accessor... I propose to
>   rename it to GetLayerPair (IIRC it already happened int the past)

This is not my code, JP should have final say.

In general when there is both a setter and getter accessor, I tend to go with Get..() and
Set..().  But when there is only a getter, I often prefer to leave off the Get..() prefix.

CurLineNumber() is an example in DSN_LEXER.  So this is a preference in my code.  No
setter is pertinent, so the getter drops the Get.  There is nothing sacred about Set and
Get, other than that we've achieved some consistency.

If there is not way to Set the LayerPair, then LayerPair() would also be OK for me.


> 
> - I have 'split' the two layer in a via adding a bottomlayer member
>   instead of that nasty hack of bit-encoding them in the primary layer. 
>   It only accessor-used but it's ugly (it will come back if someone
>   subclasses the via class, anyway)

This is code that I have worked on, don't recall who originally wrote it.  If the
interface is well defined and the contract of the interface is being met, then I would say
leave the black box implementation alone, until it stops working.

> 
> - Question: is 7000 a magic number for a layer in specctra_import?
>   a define or at least a comment would be useful.

add this:

// when 7000 stops working, ask dick



> 
> - In the tooltips inside dialog_edit_module_for_Modedit.cpp smd should
>   be uppercased :D

Yes, if that's what the UI guidelines call for, go for it.

> 
> Other layer stuff is in standby until we decide what to do (i.e.
> probably until we have the new library format working)
>