On Mon 28 March 2011 at 21:28 +0200, Xavier Chantry wrote:
> On Mon, Mar 28, 2011 at 9:26 PM, Rémy Oudompheng
> <remyoudompheng@gmail.com> wrote:
> > On 2011/3/28 Rémy Oudompheng <remyoudompheng@gmail.com> wrote:
> >> Many alpm_option_get/set_*() functions already check this
> >> and set pm_errno to the right value, but not all, so
> >> this improves consistency.
> >
> > I just noticed the ASSERT..RET_ERR construct, should it be the
> > preferred way of implementing such things ?
> >
>
> I was going to say a macro would be nice for that repetitive stuff.
>
> And indeed we already have them and there is already that code :
> ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));

I just sent another patch using the macro.

Rémy.

On Mon, Mar 28, 2011 at 2:47 PM, Rémy Oudompheng
<remyoudompheng@gmail.com> wrote:
> Many alpm_option_get/set_*() functions already check this
> and set pm_errno to the right value, but not all, so
> this improves consistency.
>
> Signed-off-by: Rémy Oudompheng <remy@archlinux.org>
> ---
> +#define RET_ERR_VOID(err) do { pm_errno = (err);
> + * * * _alpm_log(PM_LOG_DEBUG, "returning error %d from %s : %s
", err, __func__, alpm_strerrorlast());
> + * * * return; } while(0)
> +
> *#define RET_ERR(err, ret) do { pm_errno = (err);
> * * * *_alpm_log(PM_LOG_DEBUG, "returning error %d from %s : %s
", err, __func__, alpm_strerrorlast());
> * * * *return(ret); } while(0)
> --

I can tell from this bit I've snipped you are patching maint, and not
master, but I did not know this right away. It helps all of us greatly
when reviewing if we know where you intend the patch to land- indicate
that below the --- lines in your patch submission, right before the
diffstat.

Otherwise, +1 from me given a bit more testing especially if we are
going to maint with this.

-Dan