You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2010/03/09 17:19:55 UTC
[PATCH] apr_hash_this_{key,klen,val}
In using the apr_hash datastructure in Subversion, we've found that we
often only want the key or value from a hash. Furthermore, casting
the various return parameters has proven cumbersome. To solve this
problem, we've introduced three helper functions to return the key,
key length, and value from a hash iterator.
We've found these functions quite useful, so I'm including a patch to
add them to APR proper. The patch is against trunk, but if possible,
I'd like to see these APIs backported to 1.4.x and 1.5.x.
Thanks,
-Hyrum
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
[obviously, had not had enough coffee]
On 3/12/2010 1:39 PM, Greg Stein wrote:
>
> We're talking about a function prototype that current says:
>
> APR_DECLARE(void) apr_hash_this(apr_hash_index_t *hi, const void **key,
> apr_ssize_t *klen, void **val);
> and add a const to a parameter, thusly:
>
> APR_DECLARE(void) apr_hash_this(const apr_hash_index_t *hi, const void **key,
> apr_ssize_t *klen, void **val);
>
>
> THAT is a compatible change.
aren't key and ssize wrong?
const void * const * key
const apr_ssize_t *klen
Is val volatile?
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/12/2010 1:39 PM, Greg Stein wrote:
>
> Of course not. I have NO IDEA what the hell you're talking about. Why
> would you even attempt to assign an "int" function return to a "char
> *" variable? And that function is declared to take a parameter which
> you didn't give it. It's just nonsense code.
Code written in the absence of sufficient caffeine, yes, sorry about that.
The illustration that I believed wouldn't compile should have been...
int oldfunc (const char ** result);
int brokefunc ()
{
char *res;
int i = oldfunc(&res);
}
As you say, this does compile, I wrongly anticipated the side effects
of adding this constness change. It says res may be modified, but that
the data res pointed to is not volatile. I thought this further suggested
that the data returned via *result remained const, but I was wrong.
But APR assures forward and backwards source compatibility within 1.x.
So anyone coding correctly for constness in 1.5 will discover their source
code is broken against 1.4.2. Doesn't this violate the versioning contract?
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Mar 12, 2010 at 11:39, William A. Rowe Jr. <wr...@rowe-clan.net> wrote:
> On 3/12/2010 5:21 AM, Greg Stein wrote:
>>
>> It is *totally* fine to add a 'const' to a parameter that was not
>> there before. That does not change the ABI whatsoever, and it will not
>> break the API for callers. It merely gives them more information at
>> compile time.
>
> int oldfunc (const char *result);
>
> int brokefunc ()
> {
> char *res = oldfunc();
> }
>
> doesn't compile on a single platform I know of.
Of course not. I have NO IDEA what the hell you're talking about. Why
would you even attempt to assign an "int" function return to a "char
*" variable? And that function is declared to take a parameter which
you didn't give it. It's just nonsense code.
> Your statement makes no sense; how does adding const'ness to char *result
> not come with source code level compatibility breakage?
We're talking about a function prototype that current says:
APR_DECLARE(void) apr_hash_this(apr_hash_index_t *hi, const void **key,
apr_ssize_t *klen, void **val);
and add a const to a parameter, thusly:
APR_DECLARE(void) apr_hash_this(const apr_hash_index_t *hi, const void **key,
apr_ssize_t *klen, void **val);
THAT is a compatible change.
There are a number of APIs in APR which could grow more const-ness in
their params.
Cheers,
-g
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/12/2010 10:50 AM, Hyrum K. Wright wrote:
>
> On Mar 12, 2010, at 10:39 AM, William A. Rowe Jr. wrote:
>
>> On 3/12/2010 5:21 AM, Greg Stein wrote:
>>>
>>> It is *totally* fine to add a 'const' to a parameter that was not
>>> there before. That does not change the ABI whatsoever, and it will not
>>> break the API for callers. It merely gives them more information at
>>> compile time.
>>
>> int oldfunc (const char *result);
>>
>> int brokefunc ()
>> {
>> char *res = oldfunc();
>> }
>>
>> doesn't compile on a single platform I know of.
>>
>> Your statement makes no sense; how does adding const'ness to char *result
>> not come with source code level compatibility breakage?
>
> I think he means it was the 'const' which was not previously present, not the parameter itself.
That is my understanding too. In this example, int oldfunc(char *result) is transformed
through the addition of a const. And the claim still makes no sense, for the reasons that
I point out above :)
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 12, 2010, at 10:39 AM, William A. Rowe Jr. wrote:
> On 3/12/2010 5:21 AM, Greg Stein wrote:
>>
>> It is *totally* fine to add a 'const' to a parameter that was not
>> there before. That does not change the ABI whatsoever, and it will not
>> break the API for callers. It merely gives them more information at
>> compile time.
>
> int oldfunc (const char *result);
>
> int brokefunc ()
> {
> char *res = oldfunc();
> }
>
> doesn't compile on a single platform I know of.
>
> Your statement makes no sense; how does adding const'ness to char *result
> not come with source code level compatibility breakage?
I think he means it was the 'const' which was not previously present, not the parameter itself.
-Hyrum
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/12/2010 5:21 AM, Greg Stein wrote:
>
> It is *totally* fine to add a 'const' to a parameter that was not
> there before. That does not change the ABI whatsoever, and it will not
> break the API for callers. It merely gives them more information at
> compile time.
int oldfunc (const char *result);
int brokefunc ()
{
char *res = oldfunc();
}
doesn't compile on a single platform I know of.
Your statement makes no sense; how does adding const'ness to char *result
not come with source code level compatibility breakage?
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 9, 2010 at 16:42, William A. Rowe Jr. <wr...@rowe-clan.net> wrote:
> On 3/9/2010 3:37 PM, Hyrum K. Wright wrote:
>>
>> I'm also planning a followup which const-ifies the apr_hash_index_t params to these functions, as well as apr_hash_this(). Thoughts?
>
> Note const'ness will only be alterable with apr 2.0 forwards.
It is *totally* fine to add a 'const' to a parameter that was not
there before. That does not change the ABI whatsoever, and it will not
break the API for callers. It merely gives them more information at
compile time.
Cheers,
-g
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/9/2010 3:37 PM, Hyrum K. Wright wrote:
>
> I'm also planning a followup which const-ifies the apr_hash_index_t params to these functions, as well as apr_hash_this(). Thoughts?
Note const'ness will only be alterable with apr 2.0 forwards.
I'm eyeballs to alligators here, so I won't have a chance to do much with
this; there are a couple other eager apr contributors looking for my time
to evaluate other issues such as the drive letter case sensitivity issue.
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Apr 7, 2010 at 4:59 PM, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
>
>
> On Thu, Mar 25, 2010 at 2:28 PM, Hyrum K. Wright
> <hy...@mail.utexas.edu> wrote:
>>
>> On Mar 9, 2010, at 4:37 PM, Hyrum K. Wright wrote:
>>
>> >
>> > On Mar 9, 2010, at 2:00 PM, William A. Rowe Jr. wrote:
>> >
>> >> On 3/9/2010 11:48 AM, Jeff Trawick wrote:
>> >>> On Tue, Mar 9, 2010 at 11:19 AM, Hyrum K. Wright
>> >>> <hy...@mail.utexas.edu> wrote:
>> >>>> In using the apr_hash datastructure in Subversion, we've found that
>> >>>> we
>> >>>> often only want the key or value from a hash. Furthermore, casting
>> >>>> the various return parameters has proven cumbersome. To solve this
>> >>>> problem, we've introduced three helper functions to return the key,
>> >>>> key length, and value from a hash iterator.
>> >>>>
>> >>>> We've found these functions quite useful, so I'm including a patch to
>> >>>> add them to APR proper.
>> >>>
>> >>> IMO these functions are a natural addition; any concerns from the
>> >>> crowd?
>> >>>
>> >>>> The patch is against trunk, but if possible,
>> >>>> I'd like to see these APIs backported to 1.4.x and 1.5.x.
>> >>>
>> >>> too late for 1.4.x
>> >>
>> >> But not 1.5 - sounds like a great idea. Only change I'd suggest is
>> >> _key_len
>> >> or even _keylen rather than _klen for an exported public function.
>> >
>> > I'm fine with that.
>> >
>> > I don't think I've got the appropriate karma to commit to APR, so
>> > whoever commits this patch can make that change (or would it be worse the
>> > hassle for me to create a new patch?).
>> >
>> > I'm also planning a followup which const-ifies the apr_hash_index_t
>> > params to these functions, as well as apr_hash_this(). Thoughts?
>>
>> Ping. It's been a while since anybody has looked at this. Any plans to
>> commit the original (or modified original) patch?
>
> Unfortunately, this still hasn't gotten any response, so I've filed it in
> the issue tracker as issue 49065:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=49065
finally committed
care to confirm (or fix) the applicability of your patch to the 1.5.x branch?
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Thu, Mar 25, 2010 at 2:28 PM, Hyrum K. Wright <
hyrum_wright@mail.utexas.edu> wrote:
>
> On Mar 9, 2010, at 4:37 PM, Hyrum K. Wright wrote:
>
> >
> > On Mar 9, 2010, at 2:00 PM, William A. Rowe Jr. wrote:
> >
> >> On 3/9/2010 11:48 AM, Jeff Trawick wrote:
> >>> On Tue, Mar 9, 2010 at 11:19 AM, Hyrum K. Wright
> >>> <hy...@mail.utexas.edu> wrote:
> >>>> In using the apr_hash datastructure in Subversion, we've found that we
> >>>> often only want the key or value from a hash. Furthermore, casting
> >>>> the various return parameters has proven cumbersome. To solve this
> >>>> problem, we've introduced three helper functions to return the key,
> >>>> key length, and value from a hash iterator.
> >>>>
> >>>> We've found these functions quite useful, so I'm including a patch to
> >>>> add them to APR proper.
> >>>
> >>> IMO these functions are a natural addition; any concerns from the
> crowd?
> >>>
> >>>> The patch is against trunk, but if possible,
> >>>> I'd like to see these APIs backported to 1.4.x and 1.5.x.
> >>>
> >>> too late for 1.4.x
> >>
> >> But not 1.5 - sounds like a great idea. Only change I'd suggest is
> _key_len
> >> or even _keylen rather than _klen for an exported public function.
> >
> > I'm fine with that.
> >
> > I don't think I've got the appropriate karma to commit to APR, so whoever
> commits this patch can make that change (or would it be worse the hassle for
> me to create a new patch?).
> >
> > I'm also planning a followup which const-ifies the apr_hash_index_t
> params to these functions, as well as apr_hash_this(). Thoughts?
>
> Ping. It's been a while since anybody has looked at this. Any plans to
> commit the original (or modified original) patch?
>
Unfortunately, this still hasn't gotten any response, so I've filed it in
the issue tracker as issue 49065:
https://issues.apache.org/bugzilla/show_bug.cgi?id=49065
Thanks,
-Hyrum
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 9, 2010, at 4:37 PM, Hyrum K. Wright wrote:
>
> On Mar 9, 2010, at 2:00 PM, William A. Rowe Jr. wrote:
>
>> On 3/9/2010 11:48 AM, Jeff Trawick wrote:
>>> On Tue, Mar 9, 2010 at 11:19 AM, Hyrum K. Wright
>>> <hy...@mail.utexas.edu> wrote:
>>>> In using the apr_hash datastructure in Subversion, we've found that we
>>>> often only want the key or value from a hash. Furthermore, casting
>>>> the various return parameters has proven cumbersome. To solve this
>>>> problem, we've introduced three helper functions to return the key,
>>>> key length, and value from a hash iterator.
>>>>
>>>> We've found these functions quite useful, so I'm including a patch to
>>>> add them to APR proper.
>>>
>>> IMO these functions are a natural addition; any concerns from the crowd?
>>>
>>>> The patch is against trunk, but if possible,
>>>> I'd like to see these APIs backported to 1.4.x and 1.5.x.
>>>
>>> too late for 1.4.x
>>
>> But not 1.5 - sounds like a great idea. Only change I'd suggest is _key_len
>> or even _keylen rather than _klen for an exported public function.
>
> I'm fine with that.
>
> I don't think I've got the appropriate karma to commit to APR, so whoever commits this patch can make that change (or would it be worse the hassle for me to create a new patch?).
>
> I'm also planning a followup which const-ifies the apr_hash_index_t params to these functions, as well as apr_hash_this(). Thoughts?
Ping. It's been a while since anybody has looked at this. Any plans to commit the original (or modified original) patch?
-Hyrum
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 9, 2010, at 2:00 PM, William A. Rowe Jr. wrote:
> On 3/9/2010 11:48 AM, Jeff Trawick wrote:
>> On Tue, Mar 9, 2010 at 11:19 AM, Hyrum K. Wright
>> <hy...@mail.utexas.edu> wrote:
>>> In using the apr_hash datastructure in Subversion, we've found that we
>>> often only want the key or value from a hash. Furthermore, casting
>>> the various return parameters has proven cumbersome. To solve this
>>> problem, we've introduced three helper functions to return the key,
>>> key length, and value from a hash iterator.
>>>
>>> We've found these functions quite useful, so I'm including a patch to
>>> add them to APR proper.
>>
>> IMO these functions are a natural addition; any concerns from the crowd?
>>
>>> The patch is against trunk, but if possible,
>>> I'd like to see these APIs backported to 1.4.x and 1.5.x.
>>
>> too late for 1.4.x
>
> But not 1.5 - sounds like a great idea. Only change I'd suggest is _key_len
> or even _keylen rather than _klen for an exported public function.
I'm fine with that.
I don't think I've got the appropriate karma to commit to APR, so whoever commits this patch can make that change (or would it be worse the hassle for me to create a new patch?).
I'm also planning a followup which const-ifies the apr_hash_index_t params to these functions, as well as apr_hash_this(). Thoughts?
-Hyrum
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 3/9/2010 11:48 AM, Jeff Trawick wrote:
> On Tue, Mar 9, 2010 at 11:19 AM, Hyrum K. Wright
> <hy...@mail.utexas.edu> wrote:
>> In using the apr_hash datastructure in Subversion, we've found that we
>> often only want the key or value from a hash. Furthermore, casting
>> the various return parameters has proven cumbersome. To solve this
>> problem, we've introduced three helper functions to return the key,
>> key length, and value from a hash iterator.
>>
>> We've found these functions quite useful, so I'm including a patch to
>> add them to APR proper.
>
> IMO these functions are a natural addition; any concerns from the crowd?
>
>> The patch is against trunk, but if possible,
>> I'd like to see these APIs backported to 1.4.x and 1.5.x.
>
> too late for 1.4.x
But not 1.5 - sounds like a great idea. Only change I'd suggest is _key_len
or even _keylen rather than _klen for an exported public function.
Bill
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2010-03-09 at 15:45 -0600, Hyrum K. Wright wrote:
> We also have a couple of hash- and array-based iteration functions
> which might be handy. I can post those if there is interest.
Yep, let's raid the SVN treasure chest :-)
--
Bojan
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 9, 2010, at 2:42 PM, Bojan Smojver wrote:
> On Tue, 2010-03-09 at 12:48 -0500, Jeff Trawick wrote:
>> IMO these functions are a natural addition; any concerns from the
>> crowd?
>
> Didn't SVN folks also have some hash sorting functions we could "borrow"
> as well? May as well get those, while we're at it :-)
Gladly. The API looks something like:
svn_sort__hash(apr_hash_t *ht,
int (*comparison_func)(const svn_sort__item_t *,
cosnt svn_sort__item_t *),
apr_pool_t *pool);
Where a svn_sort__item_t is simply the key/klen/val tuple. I'm sure these could be easily re-worked into APR variants (there is even a large "(Should this be a permanent part of APR?)" comment at the top of this set of code. :)
We also have a couple of hash- and array-based iteration functions which might be handy. I can post those if there is interest.
-Hyrum
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2010-03-09 at 12:48 -0500, Jeff Trawick wrote:
> IMO these functions are a natural addition; any concerns from the
> crowd?
Didn't SVN folks also have some hash sorting functions we could "borrow"
as well? May as well get those, while we're at it :-)
--
Bojan
Re: [PATCH] apr_hash_this_{key,klen,val}
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Mar 9, 2010 at 11:19 AM, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
> In using the apr_hash datastructure in Subversion, we've found that we
> often only want the key or value from a hash. Furthermore, casting
> the various return parameters has proven cumbersome. To solve this
> problem, we've introduced three helper functions to return the key,
> key length, and value from a hash iterator.
>
> We've found these functions quite useful, so I'm including a patch to
> add them to APR proper.
IMO these functions are a natural addition; any concerns from the crowd?
> The patch is against trunk, but if possible,
> I'd like to see these APIs backported to 1.4.x and 1.5.x.
too late for 1.4.x