svn commit: r18218 - in trunk: . subversion/libsvn_ra_serf

On Wed, 25 Jan 2006 jerenkrantztigris.org wrote:

> Modified: trunk/subversion/libsvn_ra_serf/serf.c
> Url: http://svn.collab.net/viewcvs/sv
n/trunk/subversion/libsvn_ra_serf/serf.c?rev=18218&p1=tr
unk/subversion/libsvn_ra_serf/serf.c&p2=trunk/subversion
/libsvn_ra_serf/serf.c&r1=18217&r2=18218
>
============================================================
==================
> --- trunk/subversion/libsvn_ra_serf/serf.c	(original)
> +++ trunk/subversion/libsvn_ra_serf/serf.c	Wed Jan 25
00:03:43 2006
>  -18,6 +18,8 
>
>
>
> +#include <apr_uri.h>
> +
>  #include <serf.h>
>
>  #include "svn_pools.h"
>  -55,6 +57,47 
>      return serf_ssl;
>  }
>
> +typedef struct {
> +    apr_pool_t *pool;
> +    serf_bucket_alloc_t *bkt_alloc;
> +
Indentation.

> +    serf_context_t *context;
> +    serf_ssl_context_t *ssl_context;
> +
> +    serf_connection_t *conn;
> +
> +    /* 0 or 1 */
> +    int using_ssl;

Why not use svn_boolean_t?  Are you going to document this
struct in a
later commit?
> +} serf_session_t;
> +
> +static serf_bucket_t *
> +conn_setup(apr_socket_t *sock,
> +           void *baton,
> +           apr_pool_t *pool)

Space before paren. Applies to more places.

> +{
> +    serf_bucket_t *b;

Hmmm, one-letter names often lead to confusion.  We use
"b" for "baton" in
many places. What about using a name like
"bucket"?

I know that this is initial and experimental code, so it
might feel early
for these stylistic comments. If you're planning (and
promising) to go
through and fix all style issues last, just tell me

Thanks,
//Peter

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On 1/25/06, Peter N. Lundblad <peterfamlundblad.se> wrote:
> Indentation.

Sorry.  I hate 2 space indents - I'll try to remember in the
future.  =)

> Why not use svn_boolean_t?

An int is svn_boolean_t.  =)

> Are you going to document this struct in a
> later commit?

When I complete SSL support, yes.

> > +} serf_session_t;
> > +
> > +static serf_bucket_t *
> > +conn_setup(apr_socket_t *sock,
> > +           void *baton,
> > +           apr_pool_t *pool)
>
> Space before paren. Applies to more places.

That's one I definitely won't follow as it is optional.  =)

(The GNU coding style is *awful* to read!  But, I've said
that for years here.)

> Hmmm, one-letter names often lead to confusion.  We use
"b" for "baton" in
> many places. What about using a name like
"bucket"?

Sure.  -- justin

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On 1/25/06, Justin Erenkrantz <justinerenkrantz.com> wrote:

> > Space before paren. Applies to more places.
>
> That's one I definitely won't follow as it is optional.
 =)
>
> (The GNU coding style is *awful* to read!  But, I've
said that for years here.)

Historical precedent says that since you created the
file/module, so
you're free to choose your formatting style, as long as its
internally
consistent.  (libsvn_ra_svn and mod_dav_svn have their own
formatting
conventions, after all.)

But that's just about formatting.  Regarding the use of
svn_boolean_t:
 that's about making the code itself
"subversion-y".  I would argue
that since this is a genuine subversion module, there's
still an
obligation to make use of subversion datastructures, types,
and APIs
whereever possible.  This isn't an apache module.  

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On 1/25/06, Ben Collins-Sussman <sussmanred-bean.com> wrote:

> But that's just about formatting.

Yah, yah, I know.  It's just that if I have to follow the
GNU style, I
need to vent.

See r18225.  Happy now?    --
justin

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Wed, 2006-01-25 at 08:58 -0800, Justin Erenkrantz wrote:
> On 1/25/06, Peter N. Lundblad <peterfamlundblad.se> wrote:
> > Why not use svn_boolean_t?
> 
> An int is svn_boolean_t.  =)

What kind of argument is this?  We have a boolean type, we
prefer that
it is used for boolean values, and it matters not at all
what the
definition of that type happens to be.


------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Wed, 25 Jan 2006, Ben Collins-Sussman wrote:

> On 1/25/06, Justin Erenkrantz <justinerenkrantz.com> wrote:
>
> > > Space before paren. Applies to more places.
> >
> > That's one I definitely won't follow as it is
optional.  =)
> >
> > (The GNU coding style is *awful* to read!  But,
I've said that for years here.)
>
> Historical precedent says that since you created the
file/module, so
> you're free to choose your formatting style, as long as
its internally
> consistent.  (libsvn_ra_svn and mod_dav_svn have their
own formatting
> conventions, after all.)
>
And since the file was already created with spaces... So it
was getting
inconsitent.

Then we can discuss which style to choose.  I would prefer
the style with
spaces before parens to be used, *not* because of personal
preference of
style, but because  this is what most of our code uses. I'd
like to go
*towards* consistency, not the oposite.  To me, it is really
frustrating
to work on or review a patch trying to be consistent inside
each file when
the patch spans files with both styles.

I won't go on pushing this issue, because we can spend our
lifes on more
useful stuff than style discussions and I guess this debate
has been up
before.  I just want to point out that anyones personal
preference is
rather irrelevant here. My motivation is maintainability.

Regards,
//Peter

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Wed, Jan 25, 2006 at 10:44:45PM +0100, Peter N. Lundblad
wrote:
>...
> I won't go on pushing this issue, because we can spend
our lifes on more
> useful stuff than style discussions and I guess this
debate has been up
> before.  I just want to point out that anyones personal
preference is
> rather irrelevant here. My motivation is
maintainability.

Personal preference *is* relevant. That was defined long
ago. An
author (not-really when starting a new file, but definitely
for a
whole library) can choose to use space-before-paren on
function calls,
or no-space. And it should be consistent. But the choice has
been
there since the SVN project started nearly six years ago.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Wed, Jan 25, 2006 at 12:56:10PM -0500, Greg Hudson wrote:
> On Wed, 2006-01-25 at 08:58 -0800, Justin Erenkrantz
wrote:
> > On 1/25/06, Peter N. Lundblad <peterfamlundblad.se> wrote:
> > > Why not use svn_boolean_t?
> > 
> > An int is svn_boolean_t.  =)
> 
> What kind of argument is this?  We have a boolean type,
we prefer that
> it is used for boolean values, and it matters not at
all what the
> definition of that type happens to be.

See that smiley?

Sheesh. A bit of humor, not an "argument [for
it]".

-g

-- 
Greg Stein, http://www.lyra.org/

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Wed, 25 Jan 2006, Greg Stein wrote:

> On Wed, Jan 25, 2006 at 10:44:45PM +0100, Peter N.
Lundblad wrote:
> >...
> > I won't go on pushing this issue, because we can
spend our lifes on more
> > useful stuff than style discussions and I guess
this debate has been up
> > before.  I just want to point out that anyones
personal preference is
> > rather irrelevant here. My motivation is
maintainability.
> 
> Personal preference *is* relevant. That was defined
long ago. An
> author (not-really when starting a new file, but
definitely for a
> whole library) can choose to use space-before-paren on
function calls,
> or no-space. And it should be consistent. But the
choice has been
> there since the SVN project started nearly six years
ago.

Clearly; we're still suffering from it today.  I hate the
space-before-paren style, but more of the code is written in
that
style, and like Lundblad, I'd rather keep things consistent
for easier
maintenance and a lower barrier-to-entry.
-- 

Daniel Rall

On Wed, 25 Jan 2006, Daniel Rall wrote:

> On Wed, 25 Jan 2006, Greg Stein wrote:
>
> > On Wed, Jan 25, 2006 at 10:44:45PM +0100, Peter N.
Lundblad wrote:
> > >...
> > > I won't go on pushing this issue, because we
can spend our lifes on more
> > > useful stuff than style discussions and I
guess this debate has been up
> > > before.  I just want to point out that
anyones personal preference is
> > > rather irrelevant here. My motivation is
maintainability.
> >
> > Personal preference *is* relevant. That was
defined long ago. An
> > author (not-really when starting a new file, but
definitely for a
> > whole library) can choose to use
space-before-paren on function calls,
> > or no-space. And it should be consistent. But the
choice has been
> > there since the SVN project started nearly six
years ago.
>
> Clearly; we're still suffering from it today.  I hate
the
> space-before-paren style, but more of the code is
written in that
> style, and like Lundblad, I'd rather keep things
consistent for easier
> maintenance and a lower barrier-to-entry.
>
I don't understand why we on earth want such a
"choice".  I hereby propose
that we change our policy to say that new code should follow
the
space-before-parent rule (if it is not part of existing
files or modules
where the other rule appies).  This "choice" hurts
maintainability; lets'
not make it worse.

(I said I won't put this further, but now that I think of it
I just want
this to be decided once and forever.)

Regards,
//Peter

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org