On Sun, Jan 17, 2016 at 1:17 PM, Milan Prochac (milan AT prochac DOT sk) [via geda-user AT delorie DOT com] <geda-user AT delorie DOT com>= wrote:

On 17. 1. 2016 2= 2:12, Britton Kerin (britton DOT kerin AT gmail DOT com) [via geda-user AT delorie DOT com] wrote:

I ran into a couple more small issues. You're probably already aware bu= t here they are:

* The untrue (failure) result from SavePCBWithFormat() seems totally ignore= d, it should really get an error popup or at the very least a log message s= ince this is the case where your work isn't actually saved.=C2=A0 I'= ;m not sure if it could be handled at the SavePCBWithFormat() call points o= r if it would need to be further up.=C2=A0 This isn't a problem with yo= ur branch, master has it too.=C2=A0 But it might be easier to fix in your b= ranch since it looks like you factored SavePCBWithFormat() into backup.=C2= =A0 We don' t want to fix it both places as would just make merge pain = later

We should address this as separate issue and/or improvement.=C2=A0 I do not= want change more than necessary of the original code as part of the featur= e implementation. It makes review/testing/rollback easier.

Probably a good policy.=C2=A0 I put it in launch= pad with a note that the intention is to fix it after modular formats are m= erged.=C2=A0

=C2=A0

* It looks like SavePCB() and SavePCB2() are dead code now so should be rem= oved.=C2=A0 I like to put '// FIXME: I think I'm dead' by stuff= like this but maybe you just remember

They are used by built-in PCB format plugin. SavePCB is function in origina= l PCB code and SavePCB2=C2=A0 is used to transform parameter list. Currentl= y these are the only functions which really save the PCB layout.

Ah my bad cscope fail

=
=C2=A0

* There's some scope to reduce the number of points that need to be edi= ted in fftemplate.c.=C2=A0 For example with the below only the first define= value needs to change.

=C2=A0 =C2=A0 =C2=A0#include <stdio.h>
=C2=A0 =C2=A0 =C2=A0#define FORMAT_ID foo
=C2=A0 =C2=A0 =C2=A0// Stringify the argument.=C2=A0 Note that the argument= isn't expanded.
=C2=A0 =C2=A0 =C2=A0#define STRINGIFY(arg) #arg
=C2=A0 =C2=A0 =C2=A0// First expand arg then STRINGIFY() it.
=C2=A0 =C2=A0 =C2=A0#define EXPAND_AND_STRINGIFY(arg) STRINGIFY (arg)
=C2=A0 =C2=A0 =C2=A0// Conctenate arg1 and arg2.=C2=A0 Note that the argume= nts aren't expanded.
=C2=A0 =C2=A0 =C2=A0#define CONCAT(arg1, arg2) arg1 ## arg2
=C2=A0 =C2=A0 =C2=A0// First expand arg1 and arg2, then concatenate the res= ults.
=C2=A0 =C2=A0 =C2=A0#define EXPAND_AND_CONCAT(arg1, arg2) CONCAT (arg1, arg= 2)
=C2=A0 =C2=A0 =C2=A0#define FORMAT_ID_STRING EXPAND_AND_STRINGIFY (FORMAT_I= D)
=C2=A0 =C2=A0 =C2=A0#define FORMAT_FUNC(arg) EXPAND_AND_CONCAT (arg, FORMAT= _ID)
=C2=A0 =C2=A0 =C2=A0int
=C2=A0 =C2=A0 =C2=A0FORMAT_FUNC (parse) (char *filename)
=C2=A0 =C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0printf (
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"I'm a " FORMAT_ID_S= TRING " parser parsing %s in %s!\n",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0filename,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__func__
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0);
=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
=C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0int
=C2=A0 =C2=A0 =C2=A0main (void)
=C2=A0 =C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0(FORMAT_FUNC (parse)) ("file.foo"); =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
=C2=A0 =C2=A0 =C2=A0}

It's also a good idea to use use static functions for Parse Save etc. p= rovided everything external goes through the pointers in HID_Format, in whi= ch case the function-name generating macro invocations are optional.

IMO the template is not the file which will be edited on daily basis, so it= is not worth too much effort.

Reasonable.=C2=A0 We don't want a bunch of synonymous formats anyway= of course, but ultimately it might be worth it since a number of loaders c= ould exist.
=C2=A0
Britton