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

[Jose E. Marchesi]

Hi Mohammad.

Thanks for the updated patch.  Please see my comments below.

>  void
>  pk_set_quiet_p (pk_compiler pkc, int quiet_p)
>  {
> +  pkc->status = PK_OK;
>    pkl_set_quiet_p (pkc->compiler, quiet_p);
>  }

What about setting status the last thing in the function?

>  pk_ios
>  pk_ios_cur (pk_compiler pkc)
>  {
> +  pkc->status = PK_OK;
>    return (pk_ios) ios_cur ();
>  }

Can't ios_cur fail?

>  pk_ios
>  pk_ios_search (pk_compiler pkc, const char *handler)
>  {
> +  pkc->status = PK_OK;
>    /* XXX use pkc */
>    return (pk_ios) ios_search (handler);
>  }

Ditto for ios_search.  Can't it fail?  What if an IOS having hte given
handler cannot be found?

> @@ -413,6 +433,7 @@ pk_ios_search (pk_compiler pkc, const char *handler)
>  pk_ios
>  pk_ios_search_by_id (pk_compiler pkc, int id)
>  {
> +  pkc->status = PK_OK;
>    /* XXX use pkc */
>    return (pk_ios) ios_search_by_id (id);
>  }

Likewise.

> -  return ios_open (handler, flags, set_cur_p);
> +  enum
> +  {
> +    ELEN = 8,
> +  };

You don't need an enumerated for that.

Actually, I would use a switch statement instead of a table: the later
is too elaborated IMO.  Also, we cannot assert in library code: in case
ios_open returns an error you don't know (i.e. the `default' in the
switch) just return PK_ERROR.

> +  /* NOTE keep this array in sync with errors in ios.h */
> +  static const int E[ELEN] = {
> +    PK_OK, PK_ERROR, PK_ENOMEM, PK_EEOF, PK_EINVAL, PK_EINVAL,
> +  };
> +  int id;
> +  int e; /* error */
> +
> +  if ((id = ios_open (handler, flags, set_cur_p)) >= 0)
> +    return id;
> +
> +  e = -id;
> +  if (e >= ELEN)
> +    {
> +      assert (0 && "unknown error (not enough entries in E)");
> +      e = 1; /* used in release mode */
> +    }
> +  pkc->status = E[e];
> +  return PK_IOS_INVAL;
>  }