You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2013/03/21 14:18:43 UTC
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
julianfoad@apache.org writes:
> Author: julianfoad
> Date: Wed Mar 20 21:14:59 2013
> New Revision: 1459058
> --- subversion/trunk/subversion/include/private/svn_dep_compat.h (original)
> +++ subversion/trunk/subversion/include/private/svn_dep_compat.h Wed Mar 20 21:14:59 2013
> @@ -62,6 +62,15 @@ extern "C" {
> #define apr_array_clear(arr) (arr)->nelts = 0
> #endif
>
> +/**
> + * If we don't have a recent enough APR, emulate the behavior of the
> + * apr_hash_clear() API.
> + */
> +#if !APR_VERSION_AT_LEAST(1,3,0)
> +void svn_hash__clear(struct apr_hash_t *ht);
> +#define apr_hash_clear(ht) svn_hash__clear(ht)
> +#endif
> +
> #if !APR_VERSION_AT_LEAST(1,0,0)
> #define APR_UINT64_C(val) UINT64_C(val)
> #define APR_FPROT_OS_DEFAULT APR_OS_DEFAULT
> --- subversion/trunk/subversion/libsvn_subr/hash.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/hash.c Wed Mar 20 21:14:59 2013
> @@ -508,8 +508,8 @@ svn_hash_from_cstring_keys(apr_hash_t **
> }
>
>
> -svn_error_t *
> -svn_hash__clear(apr_hash_t *hash, apr_pool_t *pool)
> +void
> +svn_hash__clear(apr_hash_t *hash)
> {
> #if APR_VERSION_AT_LEAST(1, 3, 0)
> apr_hash_clear(hash);
> @@ -518,13 +518,12 @@ svn_hash__clear(apr_hash_t *hash, apr_po
> const void *key;
> apr_ssize_t klen;
>
> - for (hi = apr_hash_first(pool, hash); hi; hi = apr_hash_next(hi))
> + for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
> {
> apr_hash_this(hi, &key, &klen, NULL);
> apr_hash_set(hash, key, klen, NULL);
> }
> #endif
> - return SVN_NO_ERROR;
> }
>
The declaration of svn_hash__clear is only present for old APR while the
definition is always visible. I get this warning:
../src2/subversion/libsvn_subr/hash.c:512:1: warning: no previous prototype for 'svn_hash__clear' [-Wmissing-prototypes]
We could fix it by moving the declaration outside the #if making
svn_hash__clear always available. Or we could wrap the definition in a
#if (dropping the call to apr_hash_clear) so it is only available when
the declaration is available.
Which is the best solution?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 21.03.2013 14:18, Philip Martin wrote:
>> julianfoad@apache.org writes:
>>> -svn_error_t *
>>> -svn_hash__clear(apr_hash_t *hash, apr_pool_t *pool)
>>> +void
>>> +svn_hash__clear(apr_hash_t *hash)
>>> {
>>> #if APR_VERSION_AT_LEAST(1, 3, 0)
>>> apr_hash_clear(hash);
>>> @@ -518,13 +518,12 @@ svn_hash__clear(apr_hash_t *hash, apr_po
>>> const void *key;
>>> apr_ssize_t klen;
>>>
>>> - for (hi = apr_hash_first(pool, hash); hi; hi = apr_hash_next(hi))
>>> + for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
>>> {
>>> apr_hash_this(hi, &key, &klen, NULL);
>>> apr_hash_set(hash, key, klen, NULL);
>>> }
>>> #endif
>>> - return SVN_NO_ERROR;
>>> }
>>>
>> The declaration of svn_hash__clear is only present for old APR while the
>> definition is always visible. I get this warning:
>>
>> ../src2/subversion/libsvn_subr/hash.c:512:1: warning: no previous prototype
>> for 'svn_hash__clear' [-Wmissing-prototypes]
>>
>> We could fix it by moving the declaration outside the #if making
>> svn_hash__clear always available. Or we could wrap the definition in a
>> #if (dropping the call to apr_hash_clear) so it is only available when
>> the declaration is available.
>>
>> Which is the best solution?
>
> Given that this is an internal API and not subject to compatibility
> across patch releases, I think we can safely make the implementation
> conditional on the same predicate.
Sounds right to me too. I'll make it so (despite cmpilato's commit in the opposite direction a few minutes ago) unless anyone very soon disagrees.
- Julian
RE: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: MARTIN PHILIP [mailto:codematters@ntlworld.com] On Behalf Of
> Philip Martin
> Sent: donderdag 21 maart 2013 09:38
> To: Julian Foad
> Cc: Branko Čibej; dev@subversion.apache.org
> Subject: Re: svn commit: r1459058 - in /subversion/trunk/subversion:
> include/private/svn_dep_compat.h include/private/svn_subr_private.h
> libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c
> svnrdump/dump_editor.c
>
> Julian Foad <ju...@btopenworld.com> writes:
>
> > Philip Martin wrote:
> >
> >> I see similar warning for svn_dbg__preamble, svn_dbg__printf,
> >> svn_dbg__print_props, svn_error__locate, svn_error__trace when
> building
> >> without -DSVN_DEBUG.
> >>
> >> The dbg stuff is declared in private/svn_debug.h so I think we could
> >> make the dbg definitions conditional as well.
> >
> > +1 -- sounds sane to me.
>
> r880483 did that but it was reverted in r880505. It seems that on
> Windows these have to be defined all the time.
>
> >
> >> The error stuff probably needs to always be built since a 3rd party
> >> using our headers with -DSVN_DEBUG will generate references to the
> >> functions. We should arrange for the prototype to be visible when
> >> compiling error.c
> >
> > +1. Move their declarations outside the '#ifdef' blocks that
> > currently enclose them, in svn_error.h?
>
> I was thinking of defining SVN_ERR__TRACING in error.c so that the
> prototypes are always visible there. I might see if I can do something
> similar to make the svn_dbg_ prototypes visible in debug.c.
All files that we add to the .def files (using our python based extractor code) must be available as public symbols when linking as shared libraries.
APR and several other projects solve this by using __declspec(dllexport) instead of .def files, so the linker decides what should be public and not... but we decided not to a long time ago.
There is an exception list for functions that we hide somewhere and with some trickery we could enable different versions for debug and release mode.
Bert
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> Philip Martin wrote:
>
>> I see similar warning for svn_dbg__preamble, svn_dbg__printf,
>> svn_dbg__print_props, svn_error__locate, svn_error__trace when building
>> without -DSVN_DEBUG.
>>
>> The dbg stuff is declared in private/svn_debug.h so I think we could
>> make the dbg definitions conditional as well.
>
> +1 -- sounds sane to me.
r880483 did that but it was reverted in r880505. It seems that on
Windows these have to be defined all the time.
>
>> The error stuff probably needs to always be built since a 3rd party
>> using our headers with -DSVN_DEBUG will generate references to the
>> functions. We should arrange for the prototype to be visible when
>> compiling error.c
>
> +1. Move their declarations outside the '#ifdef' blocks that
> currently enclose them, in svn_error.h?
I was thinking of defining SVN_ERR__TRACING in error.c so that the
prototypes are always visible there. I might see if I can do something
similar to make the svn_dbg_ prototypes visible in debug.c.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Branko Čibej <br...@wandisco.com> writes:
>>> The declaration of svn_hash__clear is only present for old APR while the
>>> definition is always visible. I get this warning:
>>>
>>> ../src2/subversion/libsvn_subr/hash.c:512:1: warning: no previous
>>> prototype for 'svn_hash__clear' [-Wmissing-prototypes]
>>>
>>> We could fix it by moving the declaration outside the #if making
>>> svn_hash__clear always available. Or we could wrap the definition in a
>>> #if (dropping the call to apr_hash_clear) so it is only available when
>>> the declaration is available.
>>>
>>> Which is the best solution?
>>
>> Given that this is an internal API and not subject to compatibility
>> across patch releases, I think we can safely make the implementation
>> conditional on the same predicate.
Done in r1459370.
> I see similar warning for svn_dbg__preamble, svn_dbg__printf,
> svn_dbg__print_props, svn_error__locate, svn_error__trace when building
> without -DSVN_DEBUG.
>
> The dbg stuff is declared in private/svn_debug.h so I think we could
> make the dbg definitions conditional as well.
+1 -- sounds sane to me.
> The error stuff probably needs to always be built since a 3rd party
> using our headers with -DSVN_DEBUG will generate references to the
> functions. We should arrange for the prototype to be visible when
> compiling error.c
+1. Move their declarations outside the '#ifdef' blocks that currently enclose them, in svn_error.h?
- Julian
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Branko Čibej <br...@wandisco.com> writes:
>>> The declaration of svn_hash__clear is only present for old APR while the
>>> definition is always visible. I get this warning:
>>>
>>> ../src2/subversion/libsvn_subr/hash.c:512:1: warning: no previous
>>> prototype for 'svn_hash__clear' [-Wmissing-prototypes]
>>>
>>> We could fix it by moving the declaration outside the #if making
>>> svn_hash__clear always available. Or we could wrap the definition in a
>>> #if (dropping the call to apr_hash_clear) so it is only available when
>>> the declaration is available.
>>>
>>> Which is the best solution?
>>
>> Given that this is an internal API and not subject to compatibility
>> across patch releases, I think we can safely make the implementation
>> conditional on the same predicate.
Done in r1459370.
> I see similar warning for svn_dbg__preamble, svn_dbg__printf,
> svn_dbg__print_props, svn_error__locate, svn_error__trace when building
> without -DSVN_DEBUG.
>
> The dbg stuff is declared in private/svn_debug.h so I think we could
> make the dbg definitions conditional as well.
+1 -- sounds sane to me.
> The error stuff probably needs to always be built since a 3rd party
> using our headers with -DSVN_DEBUG will generate references to the
> functions. We should arrange for the prototype to be visible when
> compiling error.c
+1. Move their declarations outside the '#ifdef' blocks that currently enclose them, in svn_error.h?
- Julian
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
>> The declaration of svn_hash__clear is only present for old APR while the
>> definition is always visible. I get this warning:
>>
>> ../src2/subversion/libsvn_subr/hash.c:512:1: warning: no previous prototype for 'svn_hash__clear' [-Wmissing-prototypes]
>>
>> We could fix it by moving the declaration outside the #if making
>> svn_hash__clear always available. Or we could wrap the definition in a
>> #if (dropping the call to apr_hash_clear) so it is only available when
>> the declaration is available.
>>
>> Which is the best solution?
>
> Given that this is an internal API and not subject to compatibility
> across patch releases, I think we can safely make the implementation
> conditional on the same predicate.
I see similar warning for svn_dbg__preamble, svn_dbg__printf,
svn_dbg__print_props, svn_error__locate, svn_error__trace when building
without -DSVN_DEBUG.
The dbg stuff is declared in private/svn_debug.h so I think we could
make the dbg definitions conditional as well.
The error stuff probably needs to always be built since a 3rd party
using our headers with -DSVN_DEBUG will generate references to the
functions. We should arrange for the prototype to be visible when
compiling error.c
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1459058 - in /subversion/trunk/subversion: include/private/svn_dep_compat.h
include/private/svn_subr_private.h libsvn_ra_serf/serf.c libsvn_subr/hash.c
libsvn_wc/adm_ops.c svn/notify.c svnrdump/dump_editor.c
Posted by Branko Čibej <br...@wandisco.com>.
On 21.03.2013 14:18, Philip Martin wrote:
> julianfoad@apache.org writes:
>
>> Author: julianfoad
>> Date: Wed Mar 20 21:14:59 2013
>> New Revision: 1459058
>> --- subversion/trunk/subversion/include/private/svn_dep_compat.h (original)
>> +++ subversion/trunk/subversion/include/private/svn_dep_compat.h Wed Mar 20 21:14:59 2013
>> @@ -62,6 +62,15 @@ extern "C" {
>> #define apr_array_clear(arr) (arr)->nelts = 0
>> #endif
>>
>> +/**
>> + * If we don't have a recent enough APR, emulate the behavior of the
>> + * apr_hash_clear() API.
>> + */
>> +#if !APR_VERSION_AT_LEAST(1,3,0)
>> +void svn_hash__clear(struct apr_hash_t *ht);
>> +#define apr_hash_clear(ht) svn_hash__clear(ht)
>> +#endif
>> +
>> #if !APR_VERSION_AT_LEAST(1,0,0)
>> #define APR_UINT64_C(val) UINT64_C(val)
>> #define APR_FPROT_OS_DEFAULT APR_OS_DEFAULT
>> --- subversion/trunk/subversion/libsvn_subr/hash.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/hash.c Wed Mar 20 21:14:59 2013
>> @@ -508,8 +508,8 @@ svn_hash_from_cstring_keys(apr_hash_t **
>> }
>>
>>
>> -svn_error_t *
>> -svn_hash__clear(apr_hash_t *hash, apr_pool_t *pool)
>> +void
>> +svn_hash__clear(apr_hash_t *hash)
>> {
>> #if APR_VERSION_AT_LEAST(1, 3, 0)
>> apr_hash_clear(hash);
>> @@ -518,13 +518,12 @@ svn_hash__clear(apr_hash_t *hash, apr_po
>> const void *key;
>> apr_ssize_t klen;
>>
>> - for (hi = apr_hash_first(pool, hash); hi; hi = apr_hash_next(hi))
>> + for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
>> {
>> apr_hash_this(hi, &key, &klen, NULL);
>> apr_hash_set(hash, key, klen, NULL);
>> }
>> #endif
>> - return SVN_NO_ERROR;
>> }
>>
> The declaration of svn_hash__clear is only present for old APR while the
> definition is always visible. I get this warning:
>
> ../src2/subversion/libsvn_subr/hash.c:512:1: warning: no previous prototype for 'svn_hash__clear' [-Wmissing-prototypes]
>
> We could fix it by moving the declaration outside the #if making
> svn_hash__clear always available. Or we could wrap the definition in a
> #if (dropping the call to apr_hash_clear) so it is only available when
> the declaration is available.
>
> Which is the best solution?
Given that this is an internal API and not subject to compatibility
across patch releases, I think we can safely make the implementation
conditional on the same predicate.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com