Re: [mpop-users] [PATCH 2/9] Use a single function to perform all replacements.

[Ángel González <angel@...54...>]

Martin Lambers wrote:
> Hi!
> 
> A few comments on this patch. The change is fine with me, these are
> just nitpicks (well, maybe except for the second point).
> - The change in conf.c does not seem necessary?

It avoids a compile error :)
It is there for account_t to be defined. One problem I had was that as
you are typedefing anonymous structs, I couldn't forward-declarate them.
So delivery.h now depends on pop3.h, and pop3.h on conf.h Just to make
happy a couple of prototypes.

I tested naming the structures, but not adding the header dependence
required more changes than just including the header in a couple of
places.


> - The error messages printed from expand_session_variables() should be
>   avoided: just leave trailing '%' and unsupported expansions in the
>   string.
I wanted to ensure they were treated as errors (a new expansion could
make a previously-undefined). It leaves the string unexpanded and goes
on, but prints the warning on stderr (I think I may have copied that
from some other place of the code).


> - The step-logic in expand_session_variables() looks unusual. The idea
>   is to first count how much space you need, and do the actual
>   replacement in the second step?

Exactly.


Best regards