pygccxml-development

[pygccxml-development] Function Transformation - comments

[Roman Y. <rom...@gm...>]

Good morning.

Matthias I reviewed the code and here is my comments.

0. small but important issue: in some places within code you use
    next convention: wrapperfunc or virtualfunc. Can you separate words by
'_'? Thanks.

1. code_creators.mem_fun_v_t and mem_fun_v_wrapper_t
    I think that for a time being it is better to derive another class from
calldef_t and
    calldef_wrapper_t. There are few reasons I think why it should be done
this way:
    1. safety - we can play as much as we want, without breaking existing
code
    2. you added a lot of code to the classes, I think we will better
understand the
        whole feature if it will be separated from the existing code.

   So I did it for you. I did not add the code you send.

2. I don't understand the role of substitution_manager_t, but I'd like to
comment
   on it's implementation. Why don't you use dictionary and standard Python
string
   substitution mechanism?

   "%(return_)s" % dict( return_="123" )

   Or "..." % self.__dict__ pretty simple, less code and users already knows
this
   technique.

3. I don't understand subst_t hierarchy.  For example code_manager_t derives
from
    subst_t.  subst_t class perform text substitutions, while code_manager_t
manages
    pieces of source code for a C++ function. It seems that subst_t class
should be an
    implementation details of code_manager_t class.

4. function_transformer_t completely operates on string level. It does not
even contain
    reference to declaration it works on.

I expected to see something completely different. I thought about defining
hierarchy of
"transformation" classes, then the role of function_transformer_t could be
an
aggregation of all transformations that user want to do for the function to
convert it
to another function ( 1:1 ). Obviously, user will have to write some "glue
code" to help
all these transformations to work together and we should find the right
place to it.
I thought we will define few useful transformations and let the user to
extend them.

Also, what I did not see is responsibilities and contracts. I mean you wrote
documentation
but it is not clear from the class interfaces.

I hope you will be able to explain me all these things. Please don't stop
working on the feature.
It is very important and useful. I will try to help as much as I can. Also I
will complain and ask
stupid questions all the way :-)

I just commited few changes that will allow you to continue your work.

-- 
Roman Yakovenko
C++ Python language binding
http://www.language-binding.net/

Re: [pygccxml-development] Function Transformation - comments

[Matthias B. <ba...@ir...>]

Roman Yakovenko wrote:
> 0. small but important issue: in some places within code you use
>     next convention: wrapperfunc or virtualfunc. Can you separate words 
> by '_'? Thanks.

ok

> 1. code_creators.mem_fun_v_t and mem_fun_v_wrapper_t
>     I think that for a time being it is better to derive another class 
> from calldef_t and
>     calldef_wrapper_t. There are few reasons I think why it should be 
> done this way:
>     1. safety - we can play as much as we want, without breaking 
> existing code
>     2. you added a lot of code to the classes, I think we will better 
> understand the
>         whole feature if it will be separated from the existing code.
> 
>    So I did it for you. I did not add the code you send.

I didn't do an update yet because I think this will lead to quite a few 
conflicts now (I've noticed that you modified calldef.py as well).
So how do you want me to proceed? Stick all my stuff into the 
mem_fun_v_transformed_* classes?
Are you planning on keeping those classes (transformed and 
non-transformed versions) separate permanently or is this only for as 
long as this is work in progress?

By the way, I think the "standard" way to approach this problem would 
have been to create a branch 
(http://svnbook.red-bean.com/nightly/en/svn.branchmerge.html). But the 
repository layout doesn't seem to be prepared for this option.

> 2. I don't understand the role of substitution_manager_t, but I'd like 
> to comment
>    on it's implementation. Why don't you use dictionary and standard 
> Python string
>    substitution mechanism?
> 
>    "%(return_)s" % dict( return_="123" )

I didn't use this because there are some variables that may contain an 
entire block of source code (i.e. multiple lines) and each line should 
be indented with the same depth as the variable in the template string.
Using the Python formatting functionality only the first line would have 
been indented.

> 3. I don't understand subst_t hierarchy.  For example code_manager_t 
> derives from
>     subst_t.  subst_t class perform text substitutions, while 
> code_manager_t manages
>     pieces of source code for a C++ function. It seems that subst_t 
> class should be an
>     implementation details of code_manager_t class.

You mean I should have used delegation instead of derivation?
Well, I was considering this for a moment and decided for derivation 
because...

- ...the code manager is also supposed to do substitutions but with a 
specialized set of variables. So the "is a" relationship is given.
- ...I would have to duplicate the definition of substitute() when using 
delegation.
- ...the user still has the option to attach variables right to the code 
manager and use those for substitutions (it's a rather special case but 
who knows when it might be handy). With delegation this option would 
have to be provided explicitly (with additional code).

> 4. function_transformer_t completely operates on string level. It does 
> not even contain
>     reference to declaration it works on.

Yes?
(at least in my cases, I never needed to know the declaration)

> I expected to see something completely different. I thought about 
> defining hierarchy of
> "transformation" classes, then the role of function_transformer_t could 
> be an
> aggregation of all transformations that user want to do for the function 
> to convert it
> to another function ( 1:1 ). 

The function transformers are individual and independent pieces of code. 
The aggregation is done by the substitution manager.

> Obviously, user will have to write some 
> "glue code" to help
> all these transformations to work together and we should find the right 
> place to it.

What "glue code" do you mean? As mentioned above, it's the substitution 
manager that "glues" together the individual pieces.

> Also, what I did not see is responsibilities and contracts. I mean you 
> wrote documentation
> but it is not clear from the class interfaces.

What parts of the interfaces didn't you understand?

> 1. Why do you start to count arguments from 1 and not from zero?

An index of 0 occasionally refers to the return value.

> 2. Why Output is not according to coding convention?

Well, I left this one for the time being as this is actually a class 
that belongs into the "high level" realm and the conclusion in the wiki 
on this topic was to use the Python style guide.


- Matthias -

Re: [pygccxml-development] Function Transformation - comments

[Roman Y. <rom...@gm...>]

On 9/4/06, Matthias Baas <ba...@ir...> wrote:
> > 1. code_creators.mem_fun_v_t and mem_fun_v_wrapper_t
> >     I think that for a time being it is better to derive another class
> > from calldef_t and
> >     calldef_wrapper_t. There are few reasons I think why it should be
> > done this way:
> >     1. safety - we can play as much as we want, without breaking
> > existing code
> >     2. you added a lot of code to the classes, I think we will better
> > understand the
> >         whole feature if it will be separated from the existing code.
> >
> >    So I did it for you. I did not add the code you send.
>
> I didn't do an update yet because I think this will lead to quite a few
> conflicts now (I've noticed that you modified calldef.py as well).
> So how do you want me to proceed? Stick all my stuff into the
> mem_fun_v_transformed_* classes?
> Are you planning on keeping those classes (transformed and
> non-transformed versions) separate permanently or is this only for as
> long as this is work in progress?

I don't know. Right now it seems that I will keep them forever, but
tomorrow we could
be smarter and than we will make some refactoring.

> By the way, I think the "standard" way to approach this problem would
> have been to create a branch
> (http://svnbook.red-bean.com/nightly/en/svn.branchmerge.html). But the
> repository layout doesn't seem to be prepared for this option.

You are right, it is not prepared. We will see how it works and if
needed will create a
branch.

> > 2. I don't understand the role of substitution_manager_t, but I'd like
> > to comment
> >    on it's implementation. Why don't you use dictionary and standard
> > Python string
> >    substitution mechanism?
> >
> >    "%(return_)s" % dict( return_="123" )
>
> I didn't use this because there are some variables that may contain an
> entire block of source code (i.e. multiple lines) and each line should
> be indented with the same depth as the variable in the template string.
> Using the Python formatting functionality only the first line would have
> been indented.

If this is the only reason, than I have a solution to it - start every
variable from new
line and format its code before you pass it to the dict. After this
remove duplicate lines.

> > 3. I don't understand subst_t hierarchy.  For example code_manager_t
> > derives from
> >     subst_t.  subst_t class perform text substitutions, while
> > code_manager_t manages
> >     pieces of source code for a C++ function. It seems that subst_t
> > class should be an
> >     implementation details of code_manager_t class.
>
> You mean I should have used delegation instead of derivation?
> Well, I was considering this for a moment and decided for derivation
> because...
>
> - ...the code manager is also supposed to do substitutions but with a
> specialized set of variables. So the "is a" relationship is given.

I don't argue that they both works on strings. I argue that they have different
responcibilities and thus have to be separated.

> - ...I would have to duplicate the definition of substitute() when using
> delegation.

Implementation details.

> - ...the user still has the option to attach variables right to the code
> manager and use those for substitutions (it's a rather special case but
> who knows when it might be handy).

I am a fun of "agile software development" methodology. In our case it
means next:
we don't have use case, we don't write a code.

> > 4. function_transformer_t completely operates on string level. It does
> > not even contain
> >     reference to declaration it works on.
>
> Yes?
> (at least in my cases, I never needed to know the declaration)

And the main problem with it is that user can not understand what
transformation
does. I expected function_transformation to expose "transformed_function_type".
Thus it could be very clear what is done.

> > I expected to see something completely different. I thought about
> > defining hierarchy of
> > "transformation" classes, then the role of function_transformer_t could
> > be an
> > aggregation of all transformations that user want to do for the function
> > to convert it
> > to another function ( 1:1 ).
>
> The function transformers are individual and independent pieces of code.
> The aggregation is done by the substitution manager.

I could be wrong, but it seems that you think about function_transformer_t as
argument_policy. I will try to write a document that explains my
vision of the feature.
(Not a code, I am not going to write single line of code)

Also another problem is that you mix "configuration" classes with
classes that create
a code.

> > Obviously, user will have to write some
> > "glue code" to help
> > all these transformations to work together and we should find the right
> > place to it.
>
> What "glue code" do you mean? As mentioned above, it's the substitution
> manager that "glues" together the individual pieces.

I am not sure. I will try to explain it in the document.

> > Also, what I did not see is responsibilities and contracts. I mean you
> > wrote documentation
> > but it is not clear from the class interfaces.
>
> What parts of the interfaces didn't you understand?
>
> > 1. Why do you start to count arguments from 1 and not from zero?
>
> An index of 0 occasionally refers to the return value.

Problematic: 0 argument in Python is "self", in C++ it could be
"this". This is something
you have to explain and people will have to become accustomed.

> > 2. Why Output is not according to coding convention?
>
> Well, I left this one for the time being as this is actually a class
> that belongs into the "high level" realm and the conclusion in the wiki
> on this topic was to use the Python style guide.

I don't remember that. This is not what is written here:
https://realityforge.vrsource.org/view/PyppApi/DslDiscussion

-- 
Roman Yakovenko
C++ Python language binding
http://www.language-binding.net/