Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c

[Daniel Shahaf <d....@daniel.shahaf.name>]

Philip Martin wrote on Wed, Jan 12, 2011 at 11:01:12 +0000:
> danielsh@apache.org writes:
> 
> > Author: danielsh
> > Date: Mon Jan 10 06:03:30 2011
> > New Revision: 1057088
> >
> > URL: http://svn.apache.org/viewvc?rev=1057088&view=rev
> > Log:
> > Initialize svn_atomic_t's to zero, per svn_atomic__init_once().
> 
> That is redundant, strict speaking, because static variables are
> initialised to zero if not explicitly initialised to something else.
> 

I didn't remember that.

I don't mind to revert the change if people are comfortable with our
coding style relying on remembering which kinds of variables C does or
doesn't initialize to zero when they're not explicitly initialized...
(i.e., defensive coding)

> > Modified: subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c
> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c?rev=1057088&r1=1057087&r2=1057088&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original)
> > +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Mon Jan 10 06:03:30 2011
> > @@ -54,8 +54,9 @@
> >   *       in atexit processing, at which point we are already running in
> >   *       single threaded mode.
> >   */
> > -volatile svn_atomic_t svn_ra_svn__sasl_status;
> > +volatile svn_atomic_t svn_ra_svn__sasl_status = 0;
> 
> That is a static variable, explicitly marking it static might be more
> useful than explicitly marking it 0.
> 

Actually, it's called from svnserve's cyrus_init().

Feel free to go ahead and make any change you like --- I made
a horizontal change and I don't claim to be expertly familiar with each
of the variables I touched.

> >  
> > +/* Initialized by svn_ra_svn__sasl_common_init(). */
> >  static volatile svn_atomic_t sasl_ctx_count;
> 
> That comment is wrong, sasl_ctx_count is static and so is initialised to
> 0.
> 

Well, s/Initialized/Set to non-zero/ then?  You know what I meant,
right? :)

> 
> -- 
> Philip

Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c

["C. Michael Pilato" <cm...@collab.net>]

On 01/12/2011 09:54 AM, Branko Čibej wrote:
> 's true that static (and in fact all global) storage is default-inited
> to 0 ... but there's no harm in putting the initializer there. IMO it's
> better to have it, if only for the sake of clarity.

You took the words right outta my ... Outbox.

I take a similar approach to the use of parentheses around conditionals.
Maybe you don't *need* them due to order of operations rules, but the
additional clarity is worth it to me.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: svn commit: r1057088 - in /subversion/trunk/subversion: libsvn_fs_base/bdb/env.c libsvn_ra_svn/cyrus_auth.c libsvn_subr/io.c libsvn_subr/sqlite.c

[Branko Čibej <br...@xbc.nu>]

On 12.01.2011 15:12, Daniel Shahaf wrote:
> Philip Martin wrote on Wed, Jan 12, 2011 at 11:01:12 +0000:
>> danielsh@apache.org writes:
>>
>>> Author: danielsh
>>> Date: Mon Jan 10 06:03:30 2011
>>> New Revision: 1057088
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1057088&view=rev
>>> Log:
>>> Initialize svn_atomic_t's to zero, per svn_atomic__init_once().
>> That is redundant, strict speaking, because static variables are
>> initialised to zero if not explicitly initialised to something else.
>>
> I didn't remember that.
>
> I don't mind to revert the change if people are comfortable with our
> coding style relying on remembering which kinds of variables C does or
> doesn't initialize to zero when they're not explicitly initialized...
> (i.e., defensive coding)

's true that static (and in fact all global) storage is default-inited
to 0 ... but there's no harm in putting the initializer there. IMO it's
better to have it, if only for the sake of clarity.

-- Brane