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

[Mohammad-Reza Nabipoor]

Hi, Jose.

On Tue, Nov 10, 2020 at 08:57:25PM +0100, Jose E. Marchesi wrote:
> 
> >  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?
> 

OK.

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


All `ios_cur`, `ios_search` and `ios_search_by_id` functions will return `NULL`
when the entry does not exist.
I don't think that's a failure or error. Everything in the library worked
as expected and not found the requested item.

Egeyar! WDYT?


> > -  return ios_open (handler, flags, set_cur_p);
> > +  enum
> > +  {
> > +    ELEN = 8,
> > +  };
> 
> You don't need an enumerated for that.
> 

In general I use `enum` to define constant integers (instead of using
`const int`). Because they have less limitations than `const int`s (e.g., I
can use them in cases of a `switch` statement.
Just a personal preference :)


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

I chose the table over the `switch` statement because
  1) they are shorter
  2) I thought tables are faster (which turned out to be wrong; compilers
     generate exactly the same code (at least `gcc` and `clang`)).
I'll use a `switch` statement.

I put an `assert` to increase the chance of finding the missing error code in
tests. Otherwise how can we spot an inconsistency in error codes?
It's an internal thing and we should cover all the errors; I even think
we should put an `assert` in `default` case of `switch`, too. 


What about defining all error codes in a common header file?
And use macros (or inline functions) to translate them among modules.

e.g.,

```c
#include "libpoke-errors.h"
/* Which contains the following lines (and will be distribute along the 
libpoke.h).
  #define PK_OK 0
  #define PK_ERROR 1
  #define PK_ENOMEM 2
  #define PK_EEOF 3
  #define PK_EINVAL 4
*/

#define E_IOS_OK      0  /* The operation was performed to completion,
                          in the expected way.  */
#define E_IOS_ERROR  -1  /* An unspecified error condition happened.  */
#define E_IOS_ENOMEM -4  /* Memory allocation failure.  */
#define E_IOS_EOF    -5  /* End of file / input.  */
#define E_IOS_EINVAL -6  /* Invalid argument.  */
#define E_IOS_EOPEN  -7  /* IO space is already open.  */

/* Convert errors from E_IOS_* to PK_* */
#define ERROR_IOS(e)                     \
  ({                                     \
    int _e = PK_ERROR;                   \
                                         \
    switch (e)                           \
      {                                  \
      case E_IOS_OK: _e = PK_OK;         \
      case E_IOS_ERROR: _e = PK_ERROR;   \
      case E_IOS_ENOMEM: _e = PK_ENOMEM; \
      case E_IOS_EOF: _e = PK_EEOF;      \
      case E_IOS_EINVAL: _e = PK_EINVAL; \
      case E_IOS_EOPEN: _e = PK_EINVAL;  \
      default: assert (0);               \
      }                                  \
    _e;                                  \
  })
```

WDYT?


And I think having a consistent error code everywhere is good thing (currently
some internal functions return 1 as success and some others return 0 as 
success).
Using (e.g.,) "libpoke-error.h" we can use these errors across all modules.


Regards,
Mohammad-Reza