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/03/03 16:35:46 UTC
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
> Author: philip
> Date: Thu Mar 3 15:03:42 2011
> New Revision: 1076645
>
> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
> Log:
> Wrap pointer in a baton to avoid a complier warning or cast.
I think a single cast is better than the obfuscation of wrapping a
single value in a baton. Please reconsider this change.
-Hyrum
>
> * subversion/libsvn_wc/wc_db.c
> (struct set_changelist_baton): New.
> (set_changelist_txn): Go through struct.
> (svn_wc__db_op_set_changelist): Pass baton.
>
> 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=1076645&r1=1076644&r2=1076645&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Thu Mar 3 15:03:42 2011
> @@ -3291,6 +3291,10 @@ svn_wc__db_op_modified(svn_wc__db_t *db,
> }
>
>
> +struct set_changelist_baton {
> + const char *changelist;
> +};
> +
> /* */
> static svn_error_t *
> set_changelist_txn(void *baton,
> @@ -3298,7 +3302,7 @@ set_changelist_txn(void *baton,
> const char *local_relpath,
> apr_pool_t *scratch_pool)
> {
> - const char *new_changelist = baton;
> + struct set_changelist_baton *b = baton;
> const char *existing_changelist;
> svn_sqlite__stmt_t *stmt;
> svn_boolean_t have_row;
> @@ -3315,7 +3319,7 @@ set_changelist_txn(void *baton,
> {
> /* We need to insert an ACTUAL node, but only if we're not attempting
> to remove a (non-existent) changelist. */
> - if (new_changelist == NULL)
> + if (b->changelist == NULL)
> return SVN_NO_ERROR;
>
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> @@ -3333,8 +3337,8 @@ set_changelist_txn(void *baton,
> /* We have an existing row, and it simply needs to be updated, if
> it's different. */
> if (existing_changelist
> - && new_changelist
> - && strcmp(existing_changelist, new_changelist) == 0)
> + && b->changelist
> + && strcmp(existing_changelist, b->changelist) == 0)
> return SVN_NO_ERROR;
>
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> @@ -3342,7 +3346,7 @@ set_changelist_txn(void *baton,
> }
>
> SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id, local_relpath,
> - new_changelist));
> + b->changelist));
>
> return svn_error_return(svn_sqlite__step_done(stmt));
> }
> @@ -3356,6 +3360,7 @@ svn_wc__db_op_set_changelist(svn_wc__db_
> {
> svn_wc__db_wcroot_t *wcroot;
> const char *local_relpath;
> + struct set_changelist_baton scb;
>
> SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
>
> @@ -3364,7 +3369,9 @@ svn_wc__db_op_set_changelist(svn_wc__db_
> scratch_pool, scratch_pool));
> VERIFY_USABLE_WCROOT(wcroot);
>
> - SVN_ERR(with_db_txn(wcroot, local_relpath, set_changelist_txn, changelist,
> + scb.changelist = changelist;
> +
> + SVN_ERR(with_db_txn(wcroot, local_relpath, set_changelist_txn, &scb,
> scratch_pool));
>
> /* No need to flush the parent entries; changelists were not stored in the
>
>
>
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Mar 4, 2011 at 10:34, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>...
> Reverted r1076645 and added the cast in r1078008.
Thanks Hyrum!
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Mar 3, 2011 at 12:05 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> It is a lot of crap just to avoid a single cast. It makes the code
>> more complicated than it needs to be.
>
> I don't see the cast as an improvement but I won't object if somebody
> changes it (a bit like one variable declaration per line which I think
> is crap :)
>
> I'll point out that this is application memory. If somebody were to
> inadvertently modify the code to write through the non-const pointer
> that would be a SEGV bug waiting for an application to pass read-only
> memory. Unlikely, I know.
Reverted r1076645 and added the cast in r1078008.
-Hyrum
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> It is a lot of crap just to avoid a single cast. It makes the code
> more complicated than it needs to be.
I don't see the cast as an improvement but I won't object if somebody
changes it (a bit like one variable declaration per line which I think
is crap :)
I'll point out that this is application memory. If somebody were to
inadvertently modify the code to write through the non-const pointer
that would be a SEGV bug waiting for an application to pass read-only
memory. Unlikely, I know.
--
Philip
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 3, 2011 at 11:47, Julian Foad <ju...@wandisco.com> wrote:
> On Thu, 2011-03-03 at 16:33 +0000, Philip Martin wrote:
>> Greg Stein <gs...@gmail.com> writes:
>>
>> > On Thu, Mar 3, 2011 at 10:35, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>> >> On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
>> >>> Author: philip
>> >>> Date: Thu Mar 3 15:03:42 2011
>> >>> New Revision: 1076645
>> >>>
>> >>> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
>> >>> Log:
>> >>> Wrap pointer in a baton to avoid a complier warning or cast.
>> >>
>> >> I think a single cast is better than the obfuscation of wrapping a
>> >> single value in a baton. Please reconsider this change.
>> >
>> > Agreed. I'd much rather see (void *)changelist, than all of this stuff.
>>
>> A cast that changes the type, (void *)changelist, or one that just
>> removes the qualifier, (char *)changelist? Casts lead to questions.
>
> Sure a single arg in a struct is a bit ugly. But I don't like casting
> away "const" when we don't need to, as it can be a very useful warning
> if we don't have too many false alarms.
>
> Either way is ugly. C language has these holes in it - in this case,
> passing a pointer that may or may not be "const" just isn't well
> supported - so we have to do something ugly to work around it. There's
> no clear winner here.
>
> It's hardly obfuscation to pass a structure as a baton: that is the
> *normal* pattern for passing args to such a function. Passing a casted
> pointer to a singleton argument is the special case.
It is a lot of crap just to avoid a single cast. It makes the code
more complicated than it needs to be.
-g
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Mar 3, 2011 at 10:47 AM, Julian Foad <ju...@wandisco.com> wrote:
> On Thu, 2011-03-03 at 16:33 +0000, Philip Martin wrote:
>> Greg Stein <gs...@gmail.com> writes:
>>
>> > On Thu, Mar 3, 2011 at 10:35, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>> >> On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
>> >>> Author: philip
>> >>> Date: Thu Mar 3 15:03:42 2011
>> >>> New Revision: 1076645
>> >>>
>> >>> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
>> >>> Log:
>> >>> Wrap pointer in a baton to avoid a complier warning or cast.
>> >>
>> >> I think a single cast is better than the obfuscation of wrapping a
>> >> single value in a baton. Please reconsider this change.
>> >
>> > Agreed. I'd much rather see (void *)changelist, than all of this stuff.
>>
>> A cast that changes the type, (void *)changelist, or one that just
>> removes the qualifier, (char *)changelist? Casts lead to questions.
>
> Sure a single arg in a struct is a bit ugly. But I don't like casting
> away "const" when we don't need to, as it can be a very useful warning
> if we don't have too many false alarms.
>
> Either way is ugly. C language has these holes in it - in this case,
> passing a pointer that may or may not be "const" just isn't well
> supported - so we have to do something ugly to work around it. There's
> no clear winner here.
>
> It's hardly obfuscation to pass a structure as a baton: that is the
> *normal* pattern for passing args to such a function. Passing a casted
> pointer to a singleton argument is the special case.
Sure, but it just makes the code more complex. If I were to have a
const struct as a baton, would we also wrap that in a separate
single-member struct, just to avoid casting away the const? I hope
not. And give that argument, it makes little sense to do so in this
case.
-Hyrum
Re: svn commit: r1076645 -
/subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2011-03-03 at 16:33 +0000, Philip Martin wrote:
> Greg Stein <gs...@gmail.com> writes:
>
> > On Thu, Mar 3, 2011 at 10:35, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> >> On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
> >>> Author: philip
> >>> Date: Thu Mar 3 15:03:42 2011
> >>> New Revision: 1076645
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
> >>> Log:
> >>> Wrap pointer in a baton to avoid a complier warning or cast.
> >>
> >> I think a single cast is better than the obfuscation of wrapping a
> >> single value in a baton. Please reconsider this change.
> >
> > Agreed. I'd much rather see (void *)changelist, than all of this stuff.
>
> A cast that changes the type, (void *)changelist, or one that just
> removes the qualifier, (char *)changelist? Casts lead to questions.
Sure a single arg in a struct is a bit ugly. But I don't like casting
away "const" when we don't need to, as it can be a very useful warning
if we don't have too many false alarms.
Either way is ugly. C language has these holes in it - in this case,
passing a pointer that may or may not be "const" just isn't well
supported - so we have to do something ugly to work around it. There's
no clear winner here.
It's hardly obfuscation to pass a structure as a baton: that is the
*normal* pattern for passing args to such a function. Passing a casted
pointer to a singleton argument is the special case.
- Julian
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Branko Čibej <br...@apache.org>.
On 03.03.2011 17:33, Philip Martin wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> On Thu, Mar 3, 2011 at 10:35, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>>> On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
>>>> Author: philip
>>>> Date: Thu Mar 3 15:03:42 2011
>>>> New Revision: 1076645
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
>>>> Log:
>>>> Wrap pointer in a baton to avoid a complier warning or cast.
>>> I think a single cast is better than the obfuscation of wrapping a
>>> single value in a baton. Please reconsider this change.
>> Agreed. I'd much rather see (void *)changelist, than all of this stuff.
> A cast that changes the type, (void *)changelist, or one that just
> removes the qualifier, (char *)changelist?
One that just removes the qualifier, because the subsequent cast to
void* is implicit. Inside the implementation, you can recast directly
back to const char*, since adding a const qualifier is well-defined.
> Casts lead to questions.
Yes, and in some cases, comments answer those questions better than
uncommented single-member structs that also raise questions. "Cast away
const" is not such an obscure thing that it cannot be described in a
single-line comment.
-- Brane
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Thu, Mar 3, 2011 at 10:35, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>> On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
>>> Author: philip
>>> Date: Thu Mar 3 15:03:42 2011
>>> New Revision: 1076645
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
>>> Log:
>>> Wrap pointer in a baton to avoid a complier warning or cast.
>>
>> I think a single cast is better than the obfuscation of wrapping a
>> single value in a baton. Please reconsider this change.
>
> Agreed. I'd much rather see (void *)changelist, than all of this stuff.
A cast that changes the type, (void *)changelist, or one that just
removes the qualifier, (char *)changelist? Casts lead to questions.
--
Philip
Re: svn commit: r1076645 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 3, 2011 at 10:35, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Thu, Mar 3, 2011 at 9:03 AM, <ph...@apache.org> wrote:
>> Author: philip
>> Date: Thu Mar 3 15:03:42 2011
>> New Revision: 1076645
>>
>> URL: http://svn.apache.org/viewvc?rev=1076645&view=rev
>> Log:
>> Wrap pointer in a baton to avoid a complier warning or cast.
>
> I think a single cast is better than the obfuscation of wrapping a
> single value in a baton. Please reconsider this change.
Agreed. I'd much rather see (void *)changelist, than all of this stuff.
Thanks,
-g