kicad-developers team mailing list archive

[Miguel Angel Ajo Pelayo <miguelangel@xxxxxxx>]

[sorry Dick, you'll receive it twice, I forgot the reply-all button]

>From what I saw yesterday on my investigation, we're doing that correctly,
we fetch the .h, and as the .h has the enum, swig implement that enum
values in python,

but if you look at them carefully (under the swig code), you will see that
it internally uses "int"
for that.

So where you are saying obj.SetLayer(LAYER_15)
one could say obj.SetLayer(15)

and no typecheck will show up,

anyway, the problem could appear if an intrepid user does:

obj.SetLayer(45)


That will under the hood in our C++, because swig will accept it (int is
the internal representation used for enums values),
and if somebody does layer[n] somehow with n> elementsOf(layer) , we will
have trouble.

And the proposal was to think about throwing an standard C++ exception in
cases where it can be
dangerous to receive some kind of unexpected parameter. That is, something
that will yet compile and work
even if scripting is compiled out. And it's even something that can be good
the day we're reading (for example)
from a broken file.

Anyway, as I told before we have other options, we must choose the wise one.




2012/8/9 Miguel Angel Ajo Pelayo <miguelangel@xxxxxxx>

> Dick, what we're proposing is not that bad, I mean, it's parameter
> checking into C++ functions,
> for functions that can cause trouble. This is something good (unless
> inside of functions that can cause
> high CPU load).
>
> Anyway, other option is to handle this on the scripting side (for every
> language), renaming the dangerous
> functions to any other thing, and then redefining the function
> (typechecked now), but, In my opinion, that
> can be worse, as that means that if you increase the enum type one day,
> you will have to go to the
> renamed function and change the check.
>
> Example:
>
> obj->fn(x)   (x>10 will crash)
>
>
> so we do
>
> rename obj::fn to obj::_fn
>
> and, in the proper .i file
>
> class obj:
>     def fn(x):
>          if x>10: throw exception
>          else:  self._fn(x)
>
>
>
>
> 2012/8/9 Dick Hollenbeck <dick@xxxxxxxxxxx>
>
>> On 08/09/2012 03:33 AM, Miguel Angel Ajo Pelayo wrote:
>> > :-P :)
>> >
>> > /*
>> ----------------------------------------------------------------------------
>> >  * This file was automatically generated by SWIG (http://www.swig.org).
>> >  * Version 2.0.4
>> >  *
>> >  * This file is not intended to be easily readable and contains a
>> number of
>> >  * coding conventions designed to improve portability and efficiency.
>> Do not make
>> >  * changes to this file unless you know what you are doing--modify the
>> SWIG
>> >  * interface file instead.
>> >  *
>> -----------------------------------------------------------------------------
>> */
>> >
>> >
>> > After the joke,
>> >
>> >
>> > This is one piece of C++ code that checks for PCB_FILE_T parameter of
>> LoadBoard
>> >
>> > WIGINTERN PyObject *_wrap_LoadBoard__SWIG_0(PyObject
>> *SWIGUNUSEDPARM(self), PyObject
>> > *args) {
>> >   PyObject *resultobj = 0;
>> >   wxString *arg1 = 0 ;
>> >   IO_MGR::PCB_FILE_T arg2 ;
>> >   bool temp1 = false ;
>> >   int val2 ;
>> >   int ecode2 = 0 ;
>> >   PyObject * obj0 = 0 ;
>> >   PyObject * obj1 = 0 ;
>> >   BOARD *result = 0 ;
>> >
>> >   if (!PyArg_ParseTuple(args,(char *)"OO:LoadBoard",&obj0,&obj1))
>> SWIG_fail;
>> >   {
>> >     arg1 = newWxStringFromPy(obj0);
>> >     if (arg1 == NULL) SWIG_fail;
>> >     temp1 = true;
>> >   }
>> >  *ecode2 = SWIG_AsVal_int(obj1, &val2);*
>> >   if (!SWIG_IsOK(ecode2)) {
>> >     *SWIG_exception_fail(SWIG_ArgError(ecode2), "in method '"
>> "LoadBoard" "', argument "
>> > "2"" of type '" "IO_MGR::PCB_FILE_T""'");*
>> >   }
>> >   arg2 = static_cast< IO_MGR::PCB_FILE_T >(val2);
>> >   {
>> >     try{
>> >       result = (BOARD *)LoadBoard(*arg1,arg2);
>> >     }
>> >     catch( IO_ERROR e )
>> >     {
>> >       char ExceptionError[256];
>> >       sprintf(ExceptionError, "%s\n", TO_UTF8(e.errorText) );
>> >       PyErr_SetString(PyExc_IOError,ExceptionError);
>> >       return NULL;
>> >     }
>> >     catch( ... )
>> >     {
>> >       SWIG_fail;
>> >     }
>> >
>> >
>> >
>> > As you said, it only checks that it's an "int", and it does an staic
>> cast to PCB_FILE_T,
>> >
>> >
>> > It won't be a problem in most situations (where user is dependent from
>> the ENUMS to know
>> > what he's calling),
>> > but for layer numbers, layer colors, etc, it can be a problem, since
>> user can start
>> > tweaking the parameters, so
>> > better do a compare, and throw an exception,
>> >
>> > if you tell me what kind of exception you can throw, I could set it up
>> to convert the
>> > exception back to python-land.
>> >
>> > Do we have some kind of exception for wrong parameters? It could make
>> sense now, but not
>> > for all situations, only
>> > scripting-accesible functions, where enums could be used also as
>> numbers (layer number,
>> > color number, etc..)
>>
>>
>> Ouch.
>>
>> Tread carefully.
>>
>> The value proposition (and acceptance contract on the part of C++
>> developers to tolerate
>> SWIG) was that all the magic can be done in the SWIG interface files.
>>
>> If that contract needs to be renegotiated, then lets begin the process of
>> renegotiation,
>> rather than starting immediately into a breach of contract.    I for one
>> want the ugliness
>> out of the C++ code.
>>
>> Warning, warning, warning ...
>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
>>
>
>
>
> --
>
> Miguel Angel Ajo Pelayo
> http://www.nbee.es
> +34 636 52 25 69
> skype: ajoajoajo
>



-- 

Miguel Angel Ajo Pelayo
http://www.nbee.es
+34 636 52 25 69
skype: ajoajoajo