You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vlad Georgescu <vg...@gmail.com> on 2006/08/02 18:16:41 UTC
[PATCH] Refactor the bdb atomic initialization code into a generic implementation
[[[
Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
and convert
it to a generic implementation that can be used to initialize other libraries
(*cough* Cyrus SASL *cough*). Make bdb use the new implementation.
* subversion/include/svn_atomic.h: New file.
* subversion/libsvn_subr/atomic.c: New file.
* subversion/libsvn_fs_base/bdb/env.c
Include svn_atomic.h instead of apr_atomic.h.
(svn__atomic_t,
svn__atomic_read,
svn__atomic_set,
svn__atomic_cas): These are now in subversion/include/svn_atomic.h
and have a single underscore. All calls were changed to reflect this.
(BDB_CACHE_UNINITIALIZED,
BDB_CACHE_START_INIT,
BDB_CACHE_INIT_FAILED,
BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in svn_atomic.h.
(bdb_cache_state): This is no longer declared inside an #ifdef
APR_HAS_THREADS,
and is initialized with SVN_INITIALIZED.
(svn_fs_bdb__init_cb): New function. Contains bdb-specific initialization.
(svn_fs_bdb__init): Call svn_atomic_init_once().
]]]
--
Vlad
Re: [PATCH] Refactor the bdb atomic initialization code into a generic implementation
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/8/06, Vlad Georgescu <vg...@gmail.com> wrote:
> [[[
> Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
> and convert it to a generic implementation that can be used to
> initialize other libraries (*cough* Cyrus SASL *cough*). Make bdb use
> the new implementation.
>
> * subversion/include/private/svn_atomic.h: New file.
>
> * subversion/libsvn_subr/atomic.c: New file.
>
> * subversion/libsvn_fs_base/bdb/env.c
> Include svn_atomic.h instead of apr_atomic.h.
> (svn__atomic_t,
> svn__atomic_read,
> svn__atomic_set,
> svn__atomic_cas): These are now in subversion/include/svn_atomic.h
> and have a single
> underscore. All calls were changed to reflect this.
> (BDB_CACHE_UNINITIALIZED,
> BDB_CACHE_START_INIT,
> BDB_CACHE_INIT_FAILED,
> BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in atomic.c.
> (bdb_cache_state): This is no longer declared inside an #ifdef
> APR_HAS_THREADS,
> and is no longer explicitly initialized.
> (svn_fs_bdb__init_cb): New function. Contains bdb-specific initialization.
> (svn_fs_bdb__init): Call svn_atomic_init_once().
> ]]]
This looks great to me, committed with some minor tweaks in r21016.
Thanks!
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Refactor the bdb atomic initialization code into a generic implementation
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/6/06, Philip Martin <ph...@codematters.co.uk> wrote:
> Branko Čibej <br...@xbc.nu> writes:
>
> > Vlad Georgescu wrote:
> >> (BDB_CACHE_UNINITIALIZED,
> >> BDB_CACHE_START_INIT,
> >> BDB_CACHE_INIT_FAILED,
> >> BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in
> >> svn_atomic.h.
> > I cannot for the life of me imagine why these definitions should be part
> > of Subversion's public API. In fact, they shouldn't be. They should
> > either go into svn_private_config.h, or, if that's not appropriate
> > (being a generated header), we should introduce a new _private_ header
> > for them.
> >
> > -1 on putting them in include/svn_atomic.h, they have no use outside the
> > library implementation.
The header file is now in subversion/include/private.
> We don't need all four values, but the public API does need to provide
> a mechanism to initialize static objects of type svn_atomic_t so some
> sort of SVN_ATOMIC_ONCE_INITIALIZER is needed, unless we document that
> a static zero initialisation is sufficient.
I prefer the latter option to the former. After all, globals in C are
zero-initialized by default, so it works fine even if the user doesn't
explicitly initialize the svn_atomic_t to 0.
[[[
Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
and convert it to a generic implementation that can be used to
initialize other libraries (*cough* Cyrus SASL *cough*). Make bdb use
the new implementation.
* subversion/include/private/svn_atomic.h: New file.
* subversion/libsvn_subr/atomic.c: New file.
* subversion/libsvn_fs_base/bdb/env.c
Include svn_atomic.h instead of apr_atomic.h.
(svn__atomic_t,
svn__atomic_read,
svn__atomic_set,
svn__atomic_cas): These are now in subversion/include/svn_atomic.h
and have a single
underscore. All calls were changed to reflect this.
(BDB_CACHE_UNINITIALIZED,
BDB_CACHE_START_INIT,
BDB_CACHE_INIT_FAILED,
BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in atomic.c.
(bdb_cache_state): This is no longer declared inside an #ifdef
APR_HAS_THREADS,
and is no longer explicitly initialized.
(svn_fs_bdb__init_cb): New function. Contains bdb-specific initialization.
(svn_fs_bdb__init): Call svn_atomic_init_once().
]]]
--
Vlad
Re: [PATCH] Refactor the bdb atomic initialization code into a
generic implementation
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@xbc.nu> writes:
> Vlad Georgescu wrote:
>> (BDB_CACHE_UNINITIALIZED,
>> BDB_CACHE_START_INIT,
>> BDB_CACHE_INIT_FAILED,
>> BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in
>> svn_atomic.h.
> I cannot for the life of me imagine why these definitions should be part
> of Subversion's public API. In fact, they shouldn't be. They should
> either go into svn_private_config.h, or, if that's not appropriate
> (being a generated header), we should introduce a new _private_ header
> for them.
>
> -1 on putting them in include/svn_atomic.h, they have no use outside the
> library implementation.
We don't need all four values, but the public API does need to provide
a mechanism to initialize static objects of type svn_atomic_t so some
sort of SVN_ATOMIC_ONCE_INITIALIZER is needed, unless we document that
a static zero initialisation is sufficient.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Add private include area for Subversion-private APIs used between modules
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/7/06, Branko Čibej <br...@xbc.nu> wrote:
> Vlad Georgescu wrote:
> > On 8/5/06, Daniel Rall <dl...@collab.net> wrote:
> >> > Garrett Rooney wrote:
> >> > > Perhaps a new subversion/include/private directory for such
> >> things is
> >> > > in order, for headers that are used in various libraries but should
> >> > > not be installed. That would be somewhat nicer than the current
> >> > > practice of doing something like:
> >> > >
> >> > > #include "../libsvn_subr/atomics.h"
> >>
> >> +1! Inter-module APIs aren't necessarily always public. Introduction
> >> of a private include area (APR/APR-Util do something similar) would
> >> allow us with a clear way to share non-public APIs.
> >
> > I created subversion/include/private and added
> > subversion/include/private/*.h to private-includes in build.conf, but
> > ran into a slight problem. Running autogen.sh generates a warning
> > about missing header files when it encounters includes with a path
> > component (e.g. #include "private/svn_atomic.h"), if the path isn't
> > relative to the current directory:
> >
> > Creating build-outputs.mk...
> > WARNING: "private/svn_atomic.h" header not found, file
> > subversion/libsvn_subr/atomic.c
> > WARNING: "private/svn_atomic.h" header not found, file
> > subversion/libsvn_fs_base/bdb/env.c
> >
> > Any ideas on how to fix this? The relevant code is in
> > _scan_for_includes in gen_base.py.
> >
>
> It seems that the include scanning logic there doesn't realise that the
> preprocessor searches for "foo/bar.h" in the whole include path, not
> just the current file's directory. Try the following patch; it's not as
> generic as it could be, but it fixes this particular problem for me.
>
> [[[
> Tell the build generator about the private include area.
>
> * build.conf: Add the private include dir to private-includes.
>
> * build/generator/gen_base.py (_path_endswith): New helper function.
> (IncludeDependencyInfo._scan_for_includes):
> Account for relative path includes.
> ]]]
>
>
> Index: build.conf
> ===================================================================
> --- build.conf (revision 20993)
> +++ build.conf (working copy)
> @@ -28,6 +28,7 @@
> includes = subversion/include/*.h
> include-wildcards = *.h *.i *.swg
> private-includes =
> + subversion/include/private/*.h
> subversion/bindings/swig/include/*.swg
> subversion/libsvn_delta/compose_delta.c
> private-built-includes =
> Index: build/generator/gen_base.py
> ===================================================================
> --- build/generator/gen_base.py (revision 20993)
> +++ build/generator/gen_base.py (working copy)
> @@ -852,6 +852,18 @@
> return native_path(_re_public_include.sub(
> r"subversion/bindings/swig/proxy/\1_h.swg", build_path(fname)))
>
> +def _path_endswith(path, subpath):
> + """Check if SUBPATH is a true path suffix of PATH.
> + """
> + path_len = len(path)
> + subpath_len = len(subpath)
> +
> + return (subpath_len > 0 and path_len >= subpath_len
> + and path[-subpath_len:] == subpath
> + and (path_len == subpath_len
> + or (subpath[0] == os.sep and path[-subpath_len] == os.sep)
> + or path[-subpath_len - 1] == os.sep))
> +
> class IncludeDependencyInfo:
> """Finds all dependencies between a named set of headers, and computes
> closure, so that individual C and SWIG source files can then be scanned, and
> @@ -999,7 +1011,9 @@
> domain_fnames = self._domain.get(os.path.basename(include_param), [])
> if direct_possibility_fname in domain_fnames:
> self._upd_dep_hash(hdrs, direct_possibility_fname, type_code)
> - elif include_param.find(os.sep) == -1 and len(domain_fnames) == 1:
> + elif (len(domain_fnames) == 1
> + and (include_param.find(os.sep) == -1
> + or _path_endswith(domain_fnames[0], include_param))):
> self._upd_dep_hash(hdrs, domain_fnames[0], type_code)
> else:
> # None found
>
>
>
> -- Brane
>
>
Works great, thanks! This should be commited once the atomic patch gets in.
--
Vlad
Re: Add private include area for Subversion-private APIs used between modules
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/6/06, Branko Čibej <br...@xbc.nu> wrote:
> Vlad Georgescu wrote:
> > On 8/5/06, Daniel Rall <dl...@collab.net> wrote:
> >> > Garrett Rooney wrote:
> >> > > Perhaps a new subversion/include/private directory for such
> >> things is
> >> > > in order, for headers that are used in various libraries but should
> >> > > not be installed. That would be somewhat nicer than the current
> >> > > practice of doing something like:
> >> > >
> >> > > #include "../libsvn_subr/atomics.h"
> >>
> >> +1! Inter-module APIs aren't necessarily always public. Introduction
> >> of a private include area (APR/APR-Util do something similar) would
> >> allow us with a clear way to share non-public APIs.
> >
> > I created subversion/include/private and added
> > subversion/include/private/*.h to private-includes in build.conf, but
> > ran into a slight problem. Running autogen.sh generates a warning
> > about missing header files when it encounters includes with a path
> > component (e.g. #include "private/svn_atomic.h"), if the path isn't
> > relative to the current directory:
> >
> > Creating build-outputs.mk...
> > WARNING: "private/svn_atomic.h" header not found, file
> > subversion/libsvn_subr/atomic.c
> > WARNING: "private/svn_atomic.h" header not found, file
> > subversion/libsvn_fs_base/bdb/env.c
> >
> > Any ideas on how to fix this? The relevant code is in
> > _scan_for_includes in gen_base.py.
> >
>
> It seems that the include scanning logic there doesn't realise that the
> preprocessor searches for "foo/bar.h" in the whole include path, not
> just the current file's directory. Try the following patch; it's not as
> generic as it could be, but it fixes this particular problem for me.
>
> [[[
> Tell the build generator about the private include area.
>
> * build.conf: Add the private include dir to private-includes.
>
> * build/generator/gen_base.py (_path_endswith): New helper function.
> (IncludeDependencyInfo._scan_for_includes):
> Account for relative path includes.
> ]]]
>
>
> Index: build.conf
> ===================================================================
> --- build.conf (revision 20993)
> +++ build.conf (working copy)
> @@ -28,6 +28,7 @@
> includes = subversion/include/*.h
> include-wildcards = *.h *.i *.swg
> private-includes =
> + subversion/include/private/*.h
> subversion/bindings/swig/include/*.swg
> subversion/libsvn_delta/compose_delta.c
> private-built-includes =
> Index: build/generator/gen_base.py
> ===================================================================
> --- build/generator/gen_base.py (revision 20993)
> +++ build/generator/gen_base.py (working copy)
> @@ -852,6 +852,18 @@
> return native_path(_re_public_include.sub(
> r"subversion/bindings/swig/proxy/\1_h.swg", build_path(fname)))
>
> +def _path_endswith(path, subpath):
> + """Check if SUBPATH is a true path suffix of PATH.
> + """
> + path_len = len(path)
> + subpath_len = len(subpath)
> +
> + return (subpath_len > 0 and path_len >= subpath_len
> + and path[-subpath_len:] == subpath
> + and (path_len == subpath_len
> + or (subpath[0] == os.sep and path[-subpath_len] == os.sep)
> + or path[-subpath_len - 1] == os.sep))
> +
> class IncludeDependencyInfo:
> """Finds all dependencies between a named set of headers, and computes
> closure, so that individual C and SWIG source files can then be scanned, and
> @@ -999,7 +1011,9 @@
> domain_fnames = self._domain.get(os.path.basename(include_param), [])
> if direct_possibility_fname in domain_fnames:
> self._upd_dep_hash(hdrs, direct_possibility_fname, type_code)
> - elif include_param.find(os.sep) == -1 and len(domain_fnames) == 1:
> + elif (len(domain_fnames) == 1
> + and (include_param.find(os.sep) == -1
> + or _path_endswith(domain_fnames[0], include_param))):
> self._upd_dep_hash(hdrs, domain_fnames[0], type_code)
> else:
> # None found
Since I'm looking at committing Vlad's patch for the atomic stuff and
this is a prerequisite for it, I just committed the gen_base.py part
of this.
-garrett
Re: Add private include area for Subversion-private APIs used between
modules
Posted by Branko Čibej <br...@xbc.nu>.
Vlad Georgescu wrote:
> On 8/5/06, Daniel Rall <dl...@collab.net> wrote:
>> > Garrett Rooney wrote:
>> > > Perhaps a new subversion/include/private directory for such
>> things is
>> > > in order, for headers that are used in various libraries but should
>> > > not be installed. That would be somewhat nicer than the current
>> > > practice of doing something like:
>> > >
>> > > #include "../libsvn_subr/atomics.h"
>>
>> +1! Inter-module APIs aren't necessarily always public. Introduction
>> of a private include area (APR/APR-Util do something similar) would
>> allow us with a clear way to share non-public APIs.
>
> I created subversion/include/private and added
> subversion/include/private/*.h to private-includes in build.conf, but
> ran into a slight problem. Running autogen.sh generates a warning
> about missing header files when it encounters includes with a path
> component (e.g. #include "private/svn_atomic.h"), if the path isn't
> relative to the current directory:
>
> Creating build-outputs.mk...
> WARNING: "private/svn_atomic.h" header not found, file
> subversion/libsvn_subr/atomic.c
> WARNING: "private/svn_atomic.h" header not found, file
> subversion/libsvn_fs_base/bdb/env.c
>
> Any ideas on how to fix this? The relevant code is in
> _scan_for_includes in gen_base.py.
>
It seems that the include scanning logic there doesn't realise that the
preprocessor searches for "foo/bar.h" in the whole include path, not
just the current file's directory. Try the following patch; it's not as
generic as it could be, but it fixes this particular problem for me.
[[[
Tell the build generator about the private include area.
* build.conf: Add the private include dir to private-includes.
* build/generator/gen_base.py (_path_endswith): New helper function.
(IncludeDependencyInfo._scan_for_includes):
Account for relative path includes.
]]]
Index: build.conf
===================================================================
--- build.conf (revision 20993)
+++ build.conf (working copy)
@@ -28,6 +28,7 @@
includes = subversion/include/*.h
include-wildcards = *.h *.i *.swg
private-includes =
+ subversion/include/private/*.h
subversion/bindings/swig/include/*.swg
subversion/libsvn_delta/compose_delta.c
private-built-includes =
Index: build/generator/gen_base.py
===================================================================
--- build/generator/gen_base.py (revision 20993)
+++ build/generator/gen_base.py (working copy)
@@ -852,6 +852,18 @@
return native_path(_re_public_include.sub(
r"subversion/bindings/swig/proxy/\1_h.swg", build_path(fname)))
+def _path_endswith(path, subpath):
+ """Check if SUBPATH is a true path suffix of PATH.
+ """
+ path_len = len(path)
+ subpath_len = len(subpath)
+
+ return (subpath_len > 0 and path_len >= subpath_len
+ and path[-subpath_len:] == subpath
+ and (path_len == subpath_len
+ or (subpath[0] == os.sep and path[-subpath_len] == os.sep)
+ or path[-subpath_len - 1] == os.sep))
+
class IncludeDependencyInfo:
"""Finds all dependencies between a named set of headers, and computes
closure, so that individual C and SWIG source files can then be scanned, and
@@ -999,7 +1011,9 @@
domain_fnames = self._domain.get(os.path.basename(include_param), [])
if direct_possibility_fname in domain_fnames:
self._upd_dep_hash(hdrs, direct_possibility_fname, type_code)
- elif include_param.find(os.sep) == -1 and len(domain_fnames) == 1:
+ elif (len(domain_fnames) == 1
+ and (include_param.find(os.sep) == -1
+ or _path_endswith(domain_fnames[0], include_param))):
self._upd_dep_hash(hdrs, domain_fnames[0], type_code)
else:
# None found
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Add private include area for Subversion-private APIs used between modules
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/5/06, Daniel Rall <dl...@collab.net> wrote:
> > Garrett Rooney wrote:
> > > Perhaps a new subversion/include/private directory for such things is
> > > in order, for headers that are used in various libraries but should
> > > not be installed. That would be somewhat nicer than the current
> > > practice of doing something like:
> > >
> > > #include "../libsvn_subr/atomics.h"
>
> +1! Inter-module APIs aren't necessarily always public. Introduction
> of a private include area (APR/APR-Util do something similar) would
> allow us with a clear way to share non-public APIs.
I created subversion/include/private and added
subversion/include/private/*.h to private-includes in build.conf, but
ran into a slight problem. Running autogen.sh generates a warning
about missing header files when it encounters includes with a path
component (e.g. #include "private/svn_atomic.h"), if the path isn't
relative to the current directory:
Creating build-outputs.mk...
WARNING: "private/svn_atomic.h" header not found, file
subversion/libsvn_subr/atomic.c
WARNING: "private/svn_atomic.h" header not found, file
subversion/libsvn_fs_base/bdb/env.c
Any ideas on how to fix this? The relevant code is in
_scan_for_includes in gen_base.py.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Add private include area for Subversion-private APIs used between modules
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 04 Aug 2006, Branko Čibej wrote:
> Garrett Rooney wrote:
> > On 8/4/06, Branko Čibej <br...@xbc.nu> wrote:
> >> Vlad Georgescu wrote:
> >> > [[[
> >> > Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
> >> > and convert
> >> > it to a generic implementation that can be used to initialize other
> >> > libraries
> >> > (*cough* Cyrus SASL *cough*). Make bdb use the new implementation.
> >> >
> >> > * subversion/include/svn_atomic.h: New file.
> >> >
> >> > * subversion/libsvn_subr/atomic.c: New file.
> >> >
> >> > * subversion/libsvn_fs_base/bdb/env.c
> >> > Include svn_atomic.h instead of apr_atomic.h.
> >> > (svn__atomic_t,
> >> > svn__atomic_read,
> >> > svn__atomic_set,
> >> > svn__atomic_cas): These are now in subversion/include/svn_atomic.h
> >> > and have a single underscore. All calls were changed to reflect this.
> >> > (BDB_CACHE_UNINITIALIZED,
> >> > BDB_CACHE_START_INIT,
> >> > BDB_CACHE_INIT_FAILED,
> >> > BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in
> >> > svn_atomic.h.
> >> I cannot for the life of me imagine why these definitions should be part
> >> of Subversion's public API. In fact, they shouldn't be. They should
> >> either go into svn_private_config.h, or, if that's not appropriate
> >> (being a generated header), we should introduce a new _private_ header
> >> for them.
> >>
> >> -1 on putting them in include/svn_atomic.h, they have no use outside the
> >> library implementation.
> >
> > Perhaps a new subversion/include/private directory for such things is
> > in order, for headers that are used in various libraries but should
> > not be installed. That would be somewhat nicer than the current
> > practice of doing something like:
> >
> > #include "../libsvn_subr/atomics.h"
+1! Inter-module APIs aren't necessarily always public. Introduction
of a private include area (APR/APR-Util do something similar) would
allow us with a clear way to share non-public APIs.
Re: [PATCH] Refactor the bdb atomic initialization code into a generic
implementation
Posted by Branko Čibej <br...@xbc.nu>.
Garrett Rooney wrote:
> On 8/4/06, Branko Čibej <br...@xbc.nu> wrote:
>> Vlad Georgescu wrote:
>> > [[[
>> > Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
>> > and convert
>> > it to a generic implementation that can be used to initialize other
>> > libraries
>> > (*cough* Cyrus SASL *cough*). Make bdb use the new implementation.
>> >
>> > * subversion/include/svn_atomic.h: New file.
>> >
>> > * subversion/libsvn_subr/atomic.c: New file.
>> >
>> > * subversion/libsvn_fs_base/bdb/env.c
>> > Include svn_atomic.h instead of apr_atomic.h.
>> > (svn__atomic_t,
>> > svn__atomic_read,
>> > svn__atomic_set,
>> > svn__atomic_cas): These are now in subversion/include/svn_atomic.h
>> > and have a single underscore. All calls were changed to reflect this.
>> > (BDB_CACHE_UNINITIALIZED,
>> > BDB_CACHE_START_INIT,
>> > BDB_CACHE_INIT_FAILED,
>> > BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in
>> > svn_atomic.h.
>> I cannot for the life of me imagine why these definitions should be part
>> of Subversion's public API. In fact, they shouldn't be. They should
>> either go into svn_private_config.h, or, if that's not appropriate
>> (being a generated header), we should introduce a new _private_ header
>> for them.
>>
>> -1 on putting them in include/svn_atomic.h, they have no use outside the
>> library implementation.
>
> Perhaps a new subversion/include/private directory for such things is
> in order, for headers that are used in various libraries but should
> not be installed. That would be somewhat nicer than the current
> practice of doing something like:
>
> #include "../libsvn_subr/atomics.h"
>
> Other than that problem, I think this patch is a good step forward.
Oh, I didn't mean to criticize the intent. As usual, my gentle
admonishment came over a bit stronger than intended. :)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Refactor the bdb atomic initialization code into a generic implementation
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/4/06, Branko Čibej <br...@xbc.nu> wrote:
> Vlad Georgescu wrote:
> > [[[
> > Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
> > and convert
> > it to a generic implementation that can be used to initialize other
> > libraries
> > (*cough* Cyrus SASL *cough*). Make bdb use the new implementation.
> >
> > * subversion/include/svn_atomic.h: New file.
> >
> > * subversion/libsvn_subr/atomic.c: New file.
> >
> > * subversion/libsvn_fs_base/bdb/env.c
> > Include svn_atomic.h instead of apr_atomic.h.
> > (svn__atomic_t,
> > svn__atomic_read,
> > svn__atomic_set,
> > svn__atomic_cas): These are now in subversion/include/svn_atomic.h
> > and have a single underscore. All calls were changed to reflect this.
> > (BDB_CACHE_UNINITIALIZED,
> > BDB_CACHE_START_INIT,
> > BDB_CACHE_INIT_FAILED,
> > BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in
> > svn_atomic.h.
> I cannot for the life of me imagine why these definitions should be part
> of Subversion's public API. In fact, they shouldn't be. They should
> either go into svn_private_config.h, or, if that's not appropriate
> (being a generated header), we should introduce a new _private_ header
> for them.
>
> -1 on putting them in include/svn_atomic.h, they have no use outside the
> library implementation.
Perhaps a new subversion/include/private directory for such things is
in order, for headers that are used in various libraries but should
not be installed. That would be somewhat nicer than the current
practice of doing something like:
#include "../libsvn_subr/atomics.h"
Other than that problem, I think this patch is a good step forward.
-garrett
Re: [PATCH] Refactor the bdb atomic initialization code into a generic
implementation
Posted by Branko Čibej <br...@xbc.nu>.
Vlad Georgescu wrote:
> [[[
> Extract the atomic initialization code from libsvn_fs_base/bdb/env.c
> and convert
> it to a generic implementation that can be used to initialize other
> libraries
> (*cough* Cyrus SASL *cough*). Make bdb use the new implementation.
>
> * subversion/include/svn_atomic.h: New file.
>
> * subversion/libsvn_subr/atomic.c: New file.
>
> * subversion/libsvn_fs_base/bdb/env.c
> Include svn_atomic.h instead of apr_atomic.h.
> (svn__atomic_t,
> svn__atomic_read,
> svn__atomic_set,
> svn__atomic_cas): These are now in subversion/include/svn_atomic.h
> and have a single underscore. All calls were changed to reflect this.
> (BDB_CACHE_UNINITIALIZED,
> BDB_CACHE_START_INIT,
> BDB_CACHE_INIT_FAILED,
> BDB_CACHE_INITIALIZED): Deleted. Similar definitions are now in
> svn_atomic.h.
I cannot for the life of me imagine why these definitions should be part
of Subversion's public API. In fact, they shouldn't be. They should
either go into svn_private_config.h, or, if that's not appropriate
(being a generated header), we should introduce a new _private_ header
for them.
-1 on putting them in include/svn_atomic.h, they have no use outside the
library implementation.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org