headers
Leif Middelschulte | 12 Jun 2012 02:56
Picon

GSoC C backend report 3

Hello everyone,

find a short summary of what I did during last week below.

Tasks for the week were:
 - Make proxy function types typesafety
 - Use meaningful parameter types for proxy functions
     - Create seperate typemaps for proxy/wrapper where necessary and
document them

In order to complete these tasks I had to introduce seperate typemaps
for Proxies/Wrappers.

As I've already mentioned in my previous mail, the C backend
implementation features a very mighty function that generates the
proxy _and_ the wrapper code at once.
For software maintainability reasons, I decided to not put even more
conditional code into this one mighty function, but refactor the code
before I'd introduce seperate typemaps.
The code was pretty twisted and it took me quite some time to get to a
reasonable level of readable code.
I've finished this during this short week (religious holyday thursday)
and it behaves just like before, no breaks. Also on my trunk-rebased
branch.

http://swig.svn.sourceforge.net/viewvc/swig?view=revision&revision=13164

What it used to be like:
- lots of "(Cmp(Getattr(n, "foo"), "1") == 0)"
- lots of similar conditions (combined)
(Continue reading)

Permalink | Reply |
headers
Vadim Zeitlin | 13 Jun 2012 01:01
Gravatar

Re: GSoC C backend report 3

On Tue, 12 Jun 2012 02:56:28 +0200 Leif Middelschulte <leif.middelschulte <at> gmail.com> wrote:

LM> What it used to be like:
LM> - lots of "(Cmp(Getattr(n, "foo"), "1") == 0)"
...
LM> What it's now like:
LM> - Used macros like IS_SET_TO_ONE(n, "foo")

 Hello,

 I'm really, really far from certain that this is an improvement. The old
version might have been a bit verbose but it's perfectly clear while the
new one is less so. Also, and arguably more importantly, the former version
is used everywhere in SWIG code base while the new one is only used in C
module making it inconsistent with everything else.

 And if I could live with IS_SET_TO_ONE, I really think you should remove
the IS_EQUAL(x,y) macro because using Cmp(x,y)==0 is almost as short but is
more clear and standard.

 Finally, to close this topic, this is C++, not C, so you should define
inline functions and not macros if you think they're worth having. But I
really think they're not anyhow.

LM> What it's now like:
LM> - analogue for other long conditional statements
LM> - split functionWrapper function into:
LM>   - C wrapper/proxy (doesn't use typemaps according to comments,
LM> therefore won't need split)
LM>   - C++
(Continue reading)

Permalink | Reply |
headers
Leif Middelschulte | 15 Jun 2012 00:00
Picon

Re: GSoC C backend report 3

Am Mittwoch, 13. Juni 2012 um 01:01 schrieb Vadim Zeitlin:
On Tue, 12 Jun 2012 02:56:28 +0200 Leif Middelschulte <leif.middelschulte <at> gmail.com> wrote:

LM> What it used to be like:
LM> - lots of "(Cmp(Getattr(n, "foo"), "1") == 0)"
...
LM> What it's now like:
LM> - Used macros like IS_SET_TO_ONE(n, "foo")

Hello,

I'm really, really far from certain that this is an improvement. The old
version might have been a bit verbose but it's perfectly clear while the
new one is less so.
Also, and arguably more importantly, the former version
is used everywhere in SWIG code base while the new one is only used in C
module making it inconsistent with everything else.

And if I could live with IS_SET_TO_ONE, I really think you should remove
the IS_EQUAL(x,y) macro because using Cmp(x,y)==0 is almost as short but is
more clear and standard.

Finally, to close this topic, this is C++, not C, so you should define
inline functions and not macros if you think they're worth having. But I
really think they're not anyhow.
 
Hi,

I think you're right about this. Actually I think it's very bad that I "had to" put them into the module code. 
But you have to see the context here. Let me give you an example of how code looked like:

if (!is_global && storage && Cmp(storage, "static") != 0) {
           if ((Cmp(Getattr(n, "ismember"), "1") == 0) &&
         (Cmp(nodeType(n), "constructor") != 0)) {

vs.:

   if (!is_global && storage && !IS_EQUAL(storage, "static")) {
            if (IS_SET_TO_ONE(n, "ismember") &&
                  !IS_EQUAL(nodeType(n), "constructor")) {

I think that the macros aren't as good as they could be. E.g. embedding the nodeType() call is bad. Iirc it's another reoccuring pattern, thus get a macro/function of it's own.
Imho there should be a globally known set of macros or inline functions used accross swig for such common patterns.
I think doing so will reduce the inconsitencies in style like "!Cmp()" vs. "Cmp() == 0".
Anyway, I just wanted to explain myself and will go on with the Cmp()==0 and Cmp()!=0 style and remove the macros eventually.

LM> What it's now like:
LM> - analogue for other long conditional statements
LM> - split functionWrapper function into:
LM> - C wrapper/proxy (doesn't use typemaps according to comments,
LM> therefore won't need split)
LM> - C++
LM> - wrapper
LM> - proxy

This looks good in principle although it's a bit difficult to read the
patch to see what exactly has changed.
LM> Tasks for this week:
LM> - Actually finally make use of the split and introduce typemaps for proxy
LM> - Document those typemaps analogue to the Java backend's docs
LM> [- include SWIGPROTECT's definition into default generated swig
LM> boilerplate code]

Looking forward to this, good luck!
VZ
Thanks! 
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
_______________________________________________
Swig-devel mailing list
--

Leif
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
Permalink | Reply |
headers
William S Fulton | 24 Jul 2012 08:03
Picon
Favicon
Gravatar

Re: GSoC C backend report 3

On 14/06/12 23:00, Leif Middelschulte wrote:
> Am Mittwoch, 13. Juni 2012 um 01:01 schrieb Vadim Zeitlin:
> > On Tue, 12 Jun 2012 02:56:28 +0200 Leif Middelschulte
> > <leif.middelschulte <at> gmail.com <mailto:leif.middelschulte <at> gmail.com>>
> > wrote:
> >
> > LM> What it used to be like:
> > LM> - lots of "(Cmp(Getattr(n, "foo"), "1") == 0)"
> > ...
> > LM> What it's now like:
> > LM> - Used macros like IS_SET_TO_ONE(n, "foo")
> >
> > Hello,
> >
> > I'm really, really far from certain that this is an improvement. The old
> > version might have been a bit verbose but it's perfectly clear while the
> > new one is less so.
> > Also, and arguably more importantly, the former version
> > is used everywhere in SWIG code base while the new one is only used in C
> > module making it inconsistent with everything else.
> >
> > And if I could live with IS_SET_TO_ONE, I really think you should remove
> > the IS_EQUAL(x,y) macro because using Cmp(x,y)==0 is almost as short
> > but is
> > more clear and standard.
> >
> > Finally, to close this topic, this is C++, not C, so you should define
> > inline functions and not macros if you think they're worth having. But I
> > really think they're not anyhow.
> Hi,
>
> I think you're right about this. Actually I think it's very bad that I
> "had to" put them into the module code.
> But you have to see the context here. Let me give you an example of how
> code looked like:
>
> if (!is_global && storage && Cmp(storage, "static") != 0) {
> if ((Cmp(Getattr(n, "ismember"), "1") == 0) &&
> (Cmp(nodeType(n), "constructor") != 0)) {
>
> vs.:
>
> if (!is_global && storage && !IS_EQUAL(storage, "static")) {
> if (IS_SET_TO_ONE(n, "ismember") &&
> !IS_EQUAL(nodeType(n), "constructor")) {
>
> I think that the macros aren't as good as they could be. E.g. embedding
> the nodeType() call is bad. Iirc it's another reoccuring pattern, thus
> get a macro/function of it's own.
> Imho there should be a globally known set of macros or inline functions
> used accross swig for such common patterns.
> I think doing so will reduce the inconsitencies in style like "!Cmp()"
> vs. "Cmp() == 0".
> Anyway, I just wanted to explain myself and will go on with the Cmp()==0
> and Cmp()!=0 style and remove the macros eventually.
Sorry for stepping in so late, but there is the Equal function you could 
use for string comparison. Also for attributes in a node which are 
flags, use GetFlag(n, "ismember").

William

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
Permalink | Reply |

Gmane