Re: [Help-smalltalk] Debug informations

[Gwenaël Casaccio]

Thanks for the review :-)

You can find the new (and hope last) version of the patch!

On 23/06/2013 20:16, Holger Hans Peter Freyther wrote:
> On Fri, Jun 07, 2013 at 05:15:15PM +0200, Gwenaël Casaccio wrote:
>
> Hi,
>
> so my comments from the 8th of May still apply but here is another
> round on the same patch.
>
> > +    arguments: anInteger [
> > +        <category: 'accessing'>
> > +
> > +        ^ variables copyFrom: 1 to: anInteger
> > +    ]
> What is the thought behind not storing these in two different
> collections from the beginning?

I think it produces smaller images

> > diff --git a/libgst/comp.c b/libgst/comp.c
> > index 10330e1..b6ce106 100644
> > --- a/libgst/comp.c
> > +++ b/libgst/comp.c
> > @@ -534,6 +534,9 @@ _gst_execute_statements (OOP receiverOOP, memset (&s, 0,
> > +      _gst_compiler_state->debugInfo = _gst_identity_dictionary_new 
> > (_gst_identity_dictionary_class, 6);
> > +      INC_ADD_OOP (_gst_compiler_state->debugInfo);
> > @@ -719,6 +726,9 @@ _gst_compile_method (tree_node method,
> >     _gst_push_new_scope ();
> >     selector = compute_selector (method->v_method.selectorExpr);
> >   
> > +  _gst_compiler_state->debugInfo = _gst_identity_dictionary_new (_gst_identity_dictionary_class, 6);
> > +  INC_ADD_OOP (_gst_compiler_state->debugInfo);
> how often will both be created and one of them turns out to be garbage?

a new object can be created when items will be added in the dictionary 
(ie when it grows), also
I thing it's nice to protect object to be GC'ed for a future usage (at 
least in the compiler).

> > -      if (_gst_declare_arguments (method->v_method.selectorExpr) == -1)
> > +      if ((argCount = _gst_declare_arguments (method->v_method.selectorExpr)) 
> > == -1)
> I have commented this in the previous commit. This is something to get
> wrong most of the time and I would prefer to move the assignment out of
> the if statement.
>
> > -      if (_gst_declare_temporaries (method->v_method.temporaries) == -1)
> > +      if ((tempCount = _gst_declare_temporaries 
> > (method->v_method.temporaries)) == -1)
> same here.

Fixed

> > +      if (methodOOP != _gst_nil_oop) {
> > +        INC_ADD_OOP (methodOOP);
> > +
> > +        object = new_instance_with (_gst_array_class, argCount + tempCount, 
> > &variablesOOP);
> > +        INC_ADD_OOP (variablesOOP);
> > +
> > +        args = method->v_method.selectorExpr;
> > +        i = 0;
> > +
> > +        if (args->nodeType == TREE_BINARY_EXPR)
> > +          {
> > +            object->data[0] = _gst_intern_string 
> > (args->v_expr.expression->v_list.name);
> > +            i = i + 1;
> use i instead of 0 even if they need to match right now. (and i += 1)

Fixed

> > +
> > +        new_instance (_gst_debug_information_class, &debugInfo);
> > +        INC_ADD_OOP (debugInfo);
> > +
> > +        inst_var_at_put (debugInfo, 1, variablesOOP);
> > +        _gst_identity_dictionary_at_put (_gst_compiler_state->debugInfo, 
> > methodOOP, debugInfo);
> the naming of debugInfo for both a dictionary and an instance of the debug
> information class is a bit unfortunate. Can you think of two different names?

it's renamed as debugInfoDict

> > +        inst_var_at_put (inst_var_at (methodOOP, 3), 5, 
> > _gst_compiler_state->debugInfo);
> >       }
> >   
> >     if (methodOOP != _gst_nil_oop)
> >       {
> > -      INC_ADD_OOP (methodOOP);
> I see that this moved inside into the setjmp block. I know the code too little
> to know when EXIT_COMPILATION will be called. What is the guarantee that if
> methodOOP != nil that we already INC_ADD_OOPed it?

If there is an error the compiler will return nil thus all the object 
could be GC'ed
that's not a problem.

> > +  object = new_instance_with (_gst_array_class, argCount + tempCount, 
> > &variablesOOP);
> > +  INC_ADD_OOP (variablesOOP);
> > +
> > +  for (i = 0, args = blockExpr->v_block.arguments; args != NULL; args = 
> > args->v_list.next) {
> > +    object->data[i] = _gst_intern_string (args->v_list.name);
> > +    i = i + 1;
> i += 1
>
>
> >   OOP
> > +_gst_identity_dictionary_new (OOP classOOP, int size)
> > +{
> > +  return identity_dictionary_new (classOOP, size);
> > +}
> > +
> > +OOP
> >   identity_dictionary_new (OOP classOOP, int size)
> we could just rename it? There are only two callers of it? Either do it now or 
> later
> or is there precedence of not doing these renames?
It's renamed
> > diff --git a/libgst/files.c b/libgst/files.c
> > index a7156f9..913ade1 100644
> > --- a/libgst/files.c
> > +++ b/libgst/files.c
> > @@ -290,6 +290,8 @@ static const char standard_files[] = {
> >     "PkgLoader.st\0"
> >     "DirPackage.st\0"
> >     "Autoload.st\0"
> > +
> > +  "DebugInformation.st\0"
> >   };
> oh? that is not too late? I would assume it should be close to
> CompiledMethod code?

Done

> > +
> > +"Test debug informations are generated"
> > +Object subclass: Foo [
> > +    a_1: i_1 a_2: i_2 [
> > +        | i j k |
> > +
> > +        ^ [ :a :b :c | | d e f | ]
> > +    ]
> > +]
> > +
> > +Eval [
> > +    | mth |
> > +    mth := Foo>>#'a_1:a_2:'.
> > +    (mth arguments = #(#'i_1' #'i_2')) printNl.
> > +    (mth temporaries =  #(#'i' #'j' #'k')) printNl.
> > +    ((mth blockAt: 1) arguments = #(#'a' #'b' #'c')) printNl.
> > +    ((mth blockAt: 1) temporaries =  #(#'d' #'e' #'f')) printNl.
> > +    nil
> *yeah* !!!
:-D
> One more question. Paolo had a comment in regard to compat with older
> images. I forgot the details, do you remember them? Did you already
> include his comment?
The format of the compiled method shouldn't changed, I keep the same format
I only change the MethodInfo class.

> holger

Gwen

0001-Add-support-for-DebugInformation-final.patch
Description: Text Data