You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Hyrum K Wright <hy...@hyrumwright.org> on 2011/05/09 16:32:03 UTC
Re: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
On Sun, May 8, 2011 at 3:34 AM, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Sun May 8 08:34:56 2011
> New Revision: 1100704
>
> URL: http://svn.apache.org/viewvc?rev=1100704&view=rev
> Log:
> * subversion/libsvn_wc/wc_db.c
> (scan_addition_txn,
> wclock_obtain_cb,
> svn_wc__db_wclock_obtain,
> svn_wc__db_wclock_release): Following up on r1100353, resolve a few
> warnings.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/wc_db.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1100704&r1=1100703&r2=1100704&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Sun May 8 08:34:56 2011
> @@ -8167,7 +8167,7 @@ scan_addition_txn(void *baton,
> op_depth = svn_sqlite__column_int64(stmt, 0);
> current_relpath = local_relpath;
>
> - for (i = relpath_depth(local_relpath); i > op_depth; --i)
> + for (i = (int)relpath_depth(local_relpath); i > op_depth; --i)
> {
> /* Calculate the path of the operation root */
> repos_prefix_path =
> @@ -8265,7 +8265,7 @@ scan_addition_txn(void *baton,
> op_depth = svn_sqlite__column_int64(stmt, 0);
>
> /* Skip to op_depth */
> - for (i = relpath_depth(current_relpath); i > op_depth; i--)
> + for (i = (int)relpath_depth(current_relpath); i > op_depth; i--)
> {
> /* Calculate the path of the operation root */
> repos_prefix_path =
> @@ -9570,7 +9570,7 @@ wclock_obtain_cb(void *baton,
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb, STMT_FIND_WC_LOCK));
> SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, filter));
>
> - lock_depth = relpath_depth(local_relpath);
> + lock_depth = (int)relpath_depth(local_relpath);
> max_depth = lock_depth + bt->levels_to_lock;
>
> SVN_ERR(svn_sqlite__step(&got_row, stmt));
> @@ -9641,7 +9641,7 @@ wclock_obtain_cb(void *baton,
> {
> int levels = svn_sqlite__column_int(stmt, 0);
> if (levels >= 0)
> - levels += relpath_depth(lock_relpath);
> + levels += (int)relpath_depth(lock_relpath);
>
> SVN_ERR(svn_sqlite__reset(stmt));
>
> @@ -9717,7 +9717,7 @@ svn_wc__db_wclock_obtain(svn_wc__db_t *d
> if (!steal_lock)
> {
> int i;
> - int depth = relpath_depth(local_relpath);
> + int depth = (int)relpath_depth(local_relpath);
>
> for (i = 0; i < wcroot->owned_locks->nelts; i++)
> {
> @@ -9880,7 +9880,7 @@ wclock_owns_lock(svn_boolean_t *own_lock
>
> *own_lock = FALSE;
> owned_locks = wcroot->owned_locks;
> - lock_level = relpath_depth(local_relpath);
> + lock_level = (int)relpath_depth(local_relpath);
>
> if (exact)
> {
Instead of all the casting, why don't we just change the types of the
various local variables?
-Hyrum
RE: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Hyrum K Wright [mailto:hyrum@hyrumwright.org]
> Sent: maandag 9 mei 2011 16:32
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1100704 -
> /subversion/trunk/subversion/libsvn_wc/wc_db.c
> Instead of all the casting, why don't we just change the types of the
> various local variables?
I don't think I'll live the day where normal path lengths don't fit in an 32
bit integer.
For a 32 bit signed op-depth overflow we need at least 4GB bytes in a path
name.
Bert
RE: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Hyrum K Wright [mailto:hyrum@hyrumwright.org]
> Sent: maandag 9 mei 2011 16:32
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1100704 -
> /subversion/trunk/subversion/libsvn_wc/wc_db.c
> Instead of all the casting, why don't we just change the types of the
> various local variables?
I don't think I'll live the day where normal path lengths don't fit in an 32
bit integer.
For a 32 bit signed op-depth overflow we need at least 4GB bytes in a path
name.
Bert
Re: AW: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
"Markus Schaber" <m....@3s-software.com> writes:
>> Von: Philip Martin [mailto:philip.martin@wandisco.com]
>>
>> I don't know if we can use that for bindf as I'm not sure if it would
>> handle our "t" that consumes two arguments, but until we have some
>> sort of automatic checking the fewer format letters the better.
>> While we rely on manual checking the rule "all bound integers are
>> 64bit" is easier to verify.
>
> Maybe
> http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#Function-Attr
> ibutes helps (scroll down to the "format" section). It seems that
> allows you to apply custom printf like checking.
I don't know to which bit you are referring. We already use the
checking for svn_error_createf where the formatting is printf-like, but
for svn_sqlite__bindf the formatting is sufficiently different from
printf that I can't see how the gcc stuff would work.
--
Philip
Re: AW: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
"Markus Schaber" <m....@3s-software.com> writes:
>> Von: Philip Martin [mailto:philip.martin@wandisco.com]
>>
>> I don't know if we can use that for bindf as I'm not sure if it would
>> handle our "t" that consumes two arguments, but until we have some
>> sort of automatic checking the fewer format letters the better.
>> While we rely on manual checking the rule "all bound integers are
>> 64bit" is easier to verify.
>
> Maybe
> http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#Function-Attr
> ibutes helps (scroll down to the "format" section). It seems that
> allows you to apply custom printf like checking.
I don't know to which bit you are referring. We already use the
checking for svn_error_createf where the formatting is printf-like, but
for svn_sqlite__bindf the formatting is sufficiently different from
printf that I can't see how the gcc stuff would work.
--
Philip
AW: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Markus Schaber <m....@3s-software.com>.
Hi, Philip,
> Von: Philip Martin [mailto:philip.martin@wandisco.com]
> > To do that, I would need to introduce an additional type letter into
> > the "bindf" format, because some parameters will still need to be
> > int64_t
>
> I think that is a bad idea unless there is some way to automatically
catch
> a mismatch between an integer argument and multiple integer format
> letters. We regularly made mistakes in svn_error_createf before we
added
> the gcc stuff to do printf checking.
>
> I don't know if we can use that for bindf as I'm not sure if it would
> handle our "t" that consumes two arguments, but until we have some
sort of
> automatic checking the fewer format letters the better. While we rely
on
> manual checking the rule "all bound integers are 64bit" is easier to
> verify.
Maybe
http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#Function-Attr
ibutes helps (scroll down to the "format" section).
It seems that allows you to apply custom printf like checking.
Best regards
Markus Schaber
___________________________
We software Automation.
3S-Smart Software Solutions GmbH
Markus Schaber | Developer
Memminger Str. 151 | 87439 Kempten | Germany | Tel. +49-831-54031-0 |
Fax +49-831-54031-50
Email: m.schaber@3s-software.com | Web: http://www.3s-software.com
CoDeSys internet forum: http://forum.3s-software.com
Download CoDeSys sample projects:
http://www.3s-software.com/index.shtml?sample_projects
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner |
Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
AW: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Markus Schaber <m....@3s-software.com>.
Hi, Philip,
> Von: Philip Martin [mailto:philip.martin@wandisco.com]
> > To do that, I would need to introduce an additional type letter into
> > the "bindf" format, because some parameters will still need to be
> > int64_t
>
> I think that is a bad idea unless there is some way to automatically
catch
> a mismatch between an integer argument and multiple integer format
> letters. We regularly made mistakes in svn_error_createf before we
added
> the gcc stuff to do printf checking.
>
> I don't know if we can use that for bindf as I'm not sure if it would
> handle our "t" that consumes two arguments, but until we have some
sort of
> automatic checking the fewer format letters the better. While we rely
on
> manual checking the rule "all bound integers are 64bit" is easier to
> verify.
Maybe
http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#Function-Attr
ibutes helps (scroll down to the "format" section).
It seems that allows you to apply custom printf like checking.
Best regards
Markus Schaber
___________________________
We software Automation.
3S-Smart Software Solutions GmbH
Markus Schaber | Developer
Memminger Str. 151 | 87439 Kempten | Germany | Tel. +49-831-54031-0 |
Fax +49-831-54031-50
Email: m.schaber@3s-software.com | Web: http://www.3s-software.com
CoDeSys internet forum: http://forum.3s-software.com
Download CoDeSys sample projects:
http://www.3s-software.com/index.shtml?sample_projects
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner |
Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
Re: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> Alternatively... I'd like to change all our uses of apr_int64_t for
> (wc_id, repos_id, op_depth) to plain "int". I think that would be
> better because it would only occupy one syllable of thought-space rather
> than eight syllables. And I believe there's no functional or
> programmatic reason or benefit for having these three types be int64_t.
> To do that, I would need to introduce an additional type letter into the
> "bindf" format, because some parameters will still need to be int64_t
I think that is a bad idea unless there is some way to automatically
catch a mismatch between an integer argument and multiple integer format
letters. We regularly made mistakes in svn_error_createf before we
added the gcc stuff to do printf checking.
I don't know if we can use that for bindf as I'm not sure if it would
handle our "t" that consumes two arguments, but until we have some sort
of automatic checking the fewer format letters the better. While we
rely on manual checking the rule "all bound integers are 64bit" is
easier to verify.
--
Philip
Re: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On May 9, 2011 10:48 AM, "Julian Foad" <ju...@wandisco.com> wrote:
>
> Hyrum K Wright wrote:
> > On Sun, May 8, 2011 at 3:34 AM, <rh...@apache.org> wrote:
> > > - for (i = relpath_depth(local_relpath); i > op_depth; --i)
> > > + for (i = (int)relpath_depth(local_relpath); i > op_depth; --i)
> > >
> > > - for (i = relpath_depth(current_relpath); i > op_depth; i--)
> > > + for (i = (int)relpath_depth(current_relpath); i > op_depth;
i--)
> > >
> > > - lock_depth = relpath_depth(local_relpath);
> > > + lock_depth = (int)relpath_depth(local_relpath);
> > >
> > > - levels += relpath_depth(lock_relpath);
> > > + levels += (int)relpath_depth(lock_relpath);
> > >
> > > - int depth = relpath_depth(local_relpath);
> > > + int depth = (int)relpath_depth(local_relpath);
> > >
> > > - lock_level = relpath_depth(local_relpath);
> > > + lock_level = (int)relpath_depth(local_relpath);
>
> > Instead of all the casting, why don't we just change the types of the
> > various local variables?
>
> Alternatively... I'd like to change all our uses of apr_int64_t for
> (wc_id, repos_id, op_depth) to plain "int". I think that would be
> better because it would only occupy one syllable of thought-space rather
> than eight syllables. And I believe there's no functional or
> programmatic reason or benefit for having these three types be int64_t.
>
> To do that, I would need to introduce an additional type letter into the
> "bindf" format, because some parameters will still need to be int64_t (I
> can't remember what, but I looked at this before and there were some).
apr_time_t
I think the FS might store 64-bit values.
> Note that revnum_t is not relevant because it already has its own type
> letter "r".
>
> I was thinking like this, in sequential steps:
>
> 1. In "bindf" introduce letter "I" for int64_t (in parallel with the
> current "i" which is currently for int64_t).
I'm with Philip on this. I thought about adding a "long" type a while back,
in addition to the 64-bit, but figured that would lead to more mismatches.
>...
Cheers,
-g
Re: svn commit: r1100704 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> Alternatively... I'd like to change all our uses of apr_int64_t for
> (wc_id, repos_id, op_depth) to plain "int". I think that would be
> better because it would only occupy one syllable of thought-space rather
> than eight syllables. And I believe there's no functional or
> programmatic reason or benefit for having these three types be int64_t.
> To do that, I would need to introduce an additional type letter into the
> "bindf" format, because some parameters will still need to be int64_t
I think that is a bad idea unless there is some way to automatically
catch a mismatch between an integer argument and multiple integer format
letters. We regularly made mistakes in svn_error_createf before we
added the gcc stuff to do printf checking.
I don't know if we can use that for bindf as I'm not sure if it would
handle our "t" that consumes two arguments, but until we have some sort
of automatic checking the fewer format letters the better. While we
rely on manual checking the rule "all bound integers are 64bit" is
easier to verify.
--
Philip
Re: svn commit: r1100704 -
/subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
Hyrum K Wright wrote:
> On Sun, May 8, 2011 at 3:34 AM, <rh...@apache.org> wrote:
> > - for (i = relpath_depth(local_relpath); i > op_depth; --i)
> > + for (i = (int)relpath_depth(local_relpath); i > op_depth; --i)
> >
> > - for (i = relpath_depth(current_relpath); i > op_depth; i--)
> > + for (i = (int)relpath_depth(current_relpath); i > op_depth; i--)
> >
> > - lock_depth = relpath_depth(local_relpath);
> > + lock_depth = (int)relpath_depth(local_relpath);
> >
> > - levels += relpath_depth(lock_relpath);
> > + levels += (int)relpath_depth(lock_relpath);
> >
> > - int depth = relpath_depth(local_relpath);
> > + int depth = (int)relpath_depth(local_relpath);
> >
> > - lock_level = relpath_depth(local_relpath);
> > + lock_level = (int)relpath_depth(local_relpath);
> Instead of all the casting, why don't we just change the types of the
> various local variables?
Alternatively... I'd like to change all our uses of apr_int64_t for
(wc_id, repos_id, op_depth) to plain "int". I think that would be
better because it would only occupy one syllable of thought-space rather
than eight syllables. And I believe there's no functional or
programmatic reason or benefit for having these three types be int64_t.
To do that, I would need to introduce an additional type letter into the
"bindf" format, because some parameters will still need to be int64_t (I
can't remember what, but I looked at this before and there were some).
Note that revnum_t is not relevant because it already has its own type
letter "r".
I was thinking like this, in sequential steps:
1. In "bindf" introduce letter "I" for int64_t (in parallel with the
current "i" which is currently for int64_t).
2. Change bindf's callers to use "I" for any parameters that are still
going to be using int64_t (i.e. not wc_id, repos_id, or op_depth).
3. Change the types of (wc_id, repos_id, op_depth) variables
throughout the code to "int", and simultaneously change "bindf" so type
code "i" means plain "int".
Thoughts?
- Julian
Re: svn commit: r1100704 -
/subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
Hyrum K Wright wrote:
> On Sun, May 8, 2011 at 3:34 AM, <rh...@apache.org> wrote:
> > - for (i = relpath_depth(local_relpath); i > op_depth; --i)
> > + for (i = (int)relpath_depth(local_relpath); i > op_depth; --i)
> >
> > - for (i = relpath_depth(current_relpath); i > op_depth; i--)
> > + for (i = (int)relpath_depth(current_relpath); i > op_depth; i--)
> >
> > - lock_depth = relpath_depth(local_relpath);
> > + lock_depth = (int)relpath_depth(local_relpath);
> >
> > - levels += relpath_depth(lock_relpath);
> > + levels += (int)relpath_depth(lock_relpath);
> >
> > - int depth = relpath_depth(local_relpath);
> > + int depth = (int)relpath_depth(local_relpath);
> >
> > - lock_level = relpath_depth(local_relpath);
> > + lock_level = (int)relpath_depth(local_relpath);
> Instead of all the casting, why don't we just change the types of the
> various local variables?
Alternatively... I'd like to change all our uses of apr_int64_t for
(wc_id, repos_id, op_depth) to plain "int". I think that would be
better because it would only occupy one syllable of thought-space rather
than eight syllables. And I believe there's no functional or
programmatic reason or benefit for having these three types be int64_t.
To do that, I would need to introduce an additional type letter into the
"bindf" format, because some parameters will still need to be int64_t (I
can't remember what, but I looked at this before and there were some).
Note that revnum_t is not relevant because it already has its own type
letter "r".
I was thinking like this, in sequential steps:
1. In "bindf" introduce letter "I" for int64_t (in parallel with the
current "i" which is currently for int64_t).
2. Change bindf's callers to use "I" for any parameters that are still
going to be using int64_t (i.e. not wc_id, repos_id, or op_depth).
3. Change the types of (wc_id, repos_id, op_depth) variables
throughout the code to "int", and simultaneously change "bindf" so type
code "i" means plain "int".
Thoughts?
- Julian