Re: [PATCH] Make error handling of public API consistent

[Jose E. Marchesi]

Hi Mohammad.

> First of all: This ChangeLog is awful! Do you have any idea to make it
> simpler? Or this is just fine?

I think it is fine :)

> I think I should write a unit test for all functions of public API.
> Maybe when Luca add the output capturing facility to jitter.
>
> 2. What do you think about the `eupdate` function in `libpoke.c`?
>
> 3. Is `pk_errno` implementation acceptable (esp. when `pkc == NULL`)?

See my comments below.

> diff --git a/libpoke/libpoke.c b/libpoke/libpoke.c
> index 819d99d8..9b509cf5 100644
> --- a/libpoke/libpoke.c
> +++ b/libpoke/libpoke.c
> @@ -35,11 +35,19 @@ struct pk_compiler
>    pvm vm;
>  
>    pkl_ast_node complete_type;
> +  int error;
>  };

I would prefer that field to be called `status', not `error'.  Also, it
needs a comment explaining that the field reflects the status of the
last execution of an API function, and that it is initialized to PK_OK.

>  struct pk_term_if libpoke_term_if
>  __attribute__ ((visibility ("hidden")));
>  
> +/* update the current error */
> +static inline __attribute__ ((always_inline)) int
> +eupdate (pk_compiler pkc, int e)
> +{
> +  return pkc->error = e;
> +}

I would use a macro like this instead:

#define PK_RETURN(CODE) \
  do { pkc->status = (CODE); return (CODE); } while (0)

> +int
> +pk_errno (pk_compiler pkc)
> +{
> +  if (pkc)
> +    return pkc->error;
> +  return PK_ERROR;
> +}

I think it makes sense to return PK_ERROR when pkc == NULL.  But this
should be documented in the function prototype.

>  int
>  pk_compile_file (pk_compiler pkc, const char *filename,
>                   int *exit_status)
>  {
> -  return pkl_execute_file (pkc->compiler, filename,
> -                           exit_status);
> +  return eupdate (pkc,
> +                  !pkl_execute_file (pkc->compiler, filename, exit_status));
>  }

Please do not assume that PK_OK == 0 in the code.
What about this instead:

if (!pkl_execute_file (...))
  PK_RETURN (PK_ERROR);