You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2013/10/18 15:36:53 UTC
Re: svn commit: r1532572 - in /subversion/trunk/subversion: include/private/svn_subr_private.h
libsvn_repos/config_pool.c libsvn_subr/config.c libsvn_subr/config_file.c
libsvn_subr/config_impl.h
On 16.10.2013 00:32, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue Oct 15 22:32:44 2013
> New Revision: 1532572
>
> URL: http://svn.apache.org/r1532572
> Log:
> Add support for read-only access to svn_config_t. In read-only mode,
> concurrent multi-threaded access to the same config data structure is
> safe.
> + /* Ignore write attempts to r/o configurations.
> + *
> + * Since we should never try to modify r/o data, trigger an assertion
> + * in debug mode.
> + */
> + assert(!cfg->read_only);
> + if (cfg->read_only)
> + return;
> +
> remove_expansions(cfg);
Please don't use assert like this. You're assuming that what people like
to call "release" builds are always compiled with -DNDEBUG. I've always
found that assumption to be naïve at best.
Instead, make the code depend on whether we're in maintainer mode or
not; the result will be much less ambiguous.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1532572 - in /subversion/trunk/subversion:
include/private/svn_subr_private.h libsvn_repos/config_pool.c
libsvn_subr/config.c libsvn_subr/config_file.c libsvn_subr/config_impl.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Oct 21, 2013 at 7:25 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 20.10.2013 23:26, Stefan Fuhrmann wrote:
> > On Fri, Oct 18, 2013 at 3:36 PM, Branko Čibej <brane@wandisco.com
> > <ma...@wandisco.com>> wrote:
> >
> > On 16.10.2013 00:32, stefan2@apache.org
> > <ma...@apache.org> wrote:
> >> Author: stefan2
> >> Date: Tue Oct 15 22:32:44 2013
> >> New Revision: 1532572
> >>
> >> URL: http://svn.apache.org/r1532572
> >> Log:
> >> Add support for read-only access to svn_config_t. In read-only
> mode,
> >> concurrent multi-threaded access to the same config data structure
> is
> >> safe.
> >
> >> + /* Ignore write attempts to r/o configurations.
> >> + *
> >> + * Since we should never try to modify r/o data, trigger an
> assertion
> >> + * in debug mode.
> >> + */
> >> + assert(!cfg->read_only);
> >> + if (cfg->read_only)
> >> + return;
> >> +
> >> remove_expansions(cfg);
> >
> > Please don't use assert like this. You're assuming that what
> > people like to call "release" builds are always compiled with
> > -DNDEBUG. I've always found that assumption to be naïve at best.
> >
> > Instead, make the code depend on whether we're in maintainer mode
> > or not; the result will be much less ambiguous.
> >
> >
> > From what I can see, there is no macro to test for
> > (SVN_DEBUG being more or less equivalent to DEBUG).
> >
> > Should we introduce SVN_MAINTAINER_MODE?
>
> I'd vote for
>
> #ifdef SVN_DEBUG
> SVN_ERR_ASSERT_NO_RETURN(!cfg->read_only)
> #endif
>
> It looks like the same as "#ifndef NDEBUG", but the important difference
> is that we control it independently of NDEBUG, and more importantly, we
> already use it for these kinds of checks.
>
Committed as r1534110.
-- Stefan^2.
Re: svn commit: r1532572 - in /subversion/trunk/subversion: include/private/svn_subr_private.h
libsvn_repos/config_pool.c libsvn_subr/config.c libsvn_subr/config_file.c
libsvn_subr/config_impl.h
Posted by Branko Čibej <br...@wandisco.com>.
On 20.10.2013 23:26, Stefan Fuhrmann wrote:
> On Fri, Oct 18, 2013 at 3:36 PM, Branko Čibej <brane@wandisco.com
> <ma...@wandisco.com>> wrote:
>
> On 16.10.2013 00:32, stefan2@apache.org
> <ma...@apache.org> wrote:
>> Author: stefan2
>> Date: Tue Oct 15 22:32:44 2013
>> New Revision: 1532572
>>
>> URL: http://svn.apache.org/r1532572
>> Log:
>> Add support for read-only access to svn_config_t. In read-only mode,
>> concurrent multi-threaded access to the same config data structure is
>> safe.
>
>> + /* Ignore write attempts to r/o configurations.
>> + *
>> + * Since we should never try to modify r/o data, trigger an assertion
>> + * in debug mode.
>> + */
>> + assert(!cfg->read_only);
>> + if (cfg->read_only)
>> + return;
>> +
>> remove_expansions(cfg);
>
> Please don't use assert like this. You're assuming that what
> people like to call "release" builds are always compiled with
> -DNDEBUG. I've always found that assumption to be naïve at best.
>
> Instead, make the code depend on whether we're in maintainer mode
> or not; the result will be much less ambiguous.
>
>
> From what I can see, there is no macro to test for
> (SVN_DEBUG being more or less equivalent to DEBUG).
>
> Should we introduce SVN_MAINTAINER_MODE?
I'd vote for
#ifdef SVN_DEBUG
SVN_ERR_ASSERT_NO_RETURN(!cfg->read_only)
#endif
It looks like the same as "#ifndef NDEBUG", but the important difference
is that we control it independently of NDEBUG, and more importantly, we
already use it for these kinds of checks.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1532572 - in /subversion/trunk/subversion:
include/private/svn_subr_private.h libsvn_repos/config_pool.c
libsvn_subr/config.c libsvn_subr/config_file.c libsvn_subr/config_impl.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Oct 18, 2013 at 3:36 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 16.10.2013 00:32, stefan2@apache.org wrote:
>
> Author: stefan2
> Date: Tue Oct 15 22:32:44 2013
> New Revision: 1532572
>
> URL: http://svn.apache.org/r1532572
> Log:
> Add support for read-only access to svn_config_t. In read-only mode,
> concurrent multi-threaded access to the same config data structure is
> safe.
>
>
> + /* Ignore write attempts to r/o configurations.
> + *
> + * Since we should never try to modify r/o data, trigger an assertion
> + * in debug mode.
> + */
> + assert(!cfg->read_only);
> + if (cfg->read_only)
> + return;
> +
> remove_expansions(cfg);
>
>
> Please don't use assert like this. You're assuming that what people like
> to call "release" builds are always compiled with -DNDEBUG. I've always
> found that assumption to be naïve at best.
>
> Instead, make the code depend on whether we're in maintainer mode or not;
> the result will be much less ambiguous.
>
>From what I can see, there is no macro to test for
(SVN_DEBUG being more or less equivalent to DEBUG).
Should we introduce SVN_MAINTAINER_MODE?
-- Stefan^2.