On Tue, May 10, 2016 at 03:51:04PM -0700, Kevin J. McCarthy wrote:
> On Mon, May 09, 2016 at 10:34:03PM -0400, Damien Riegel wrote:
> > Changes in v2:
> > - Addressed styling issues
> > - Got rid of the probing logic. For now, we still rely on a switch case
> > to retrieve the correct mx_ops. So, one switch case was remove and
> > another one was added, but as more functions will be added in the
> > struct mx_ops, we will get rid of more {switch,if}(ctx->magic). This
> > is only the first step!
> > - Removed patches that changed mx_fastclose_mailbox
> > - Folded similar patches into larger commits
>
> Hi Damien,
>
> Well, I think this is a good start. I'm inclined to commit this, unless
> anyone has objections.
>
> Some things I would like to see changed in your next patch:
>
> * The structs in imap.c, mbox.c, mh.c, pop.c still have
> the wrong style ("struct {" instead of "struct\n{")
A line ending with "=" just felt so weird that I left it. Anyway, fixed
it for new version.
> * I would also like to see those structs moved to near the top of the
> various files, rather than being buried in the middle of them.
> (Yes, this means you'll have to add some function prototypes for the
> static functions.)
>
> * Lastly, I'd like the
> extern struct mx_ops mx_*_ops;
> declarations moved inside mx.h, pop.h, and imap.h.
Do you mind if I move the structs to the very bottom instead? As I added
extern struct mx_ops mx_*_ops in the headers, there is no declaration
issue, and this way there is no need to add function prototypes.
This seems to be the common practice in many FOSS projects (like Linux
drivers, ffmpeg, tig...).
Thanks for the review,
--
Damien
