Re: [Help-smalltalk] Debug informations

[Holger Hans Peter Freyther]

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?


> 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?


> -      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.


> +      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)


> +
> +        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?

> +        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?



> +  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?


> 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?

> +
> +"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* !!!

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?

holger