You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2013/03/20 04:47:11 UTC
Use svn_hash_gets
See attached patch. Any objections to doing that (and similar changes
to the rest of the code)?
---
This was generated by:
1. 'spatch -sp_file 1 subversion/libsvn_wc/*.c', with the following patch:
[[[
svn-qavm,0:svn/t1% cat 1.cocci
@@
expression ht, key;
@@
-apr_hash_get(ht, key, APR_HASH_KEY_STRING)
+svn_hash_gets(ht, key)
@@
expression ht, key, val;
@@
-apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
+svn_hash_sets(ht, key, val)
]]]
2. vim, with
:set aw et ai si ci sw=2 ts=2 cin cino=>2,n2,f0,{2,}0,=2,t0,+4,C1,(0,u0,*300
:map <F3> :cn<CR>n
:map <F4> gw%
:map <F5> f,f<Space>r<CR>gw%
:grep -R svn_hash_sets subversion/
:" ### TODO: that grep should be improved to match only lines over 80 chars.
(those cinoptions don't format blocks correctly, but they suffice for
the one-line task at hand here)
Re: Use svn_hash_gets
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 20.03.2013 16:16, Julian Foad wrote:
>> C. Michael Pilato wrote:
>>
>>> Since svn_hash.h includes svn_types.h, won't this be more like
> replacing the
>>> inclusion of the latter with the inclusion of the former?
>> I'm not sure exactly what you mean, but if we do decide to leave the
> definitions in svn_hash.h and add '#include <svn_hash.h>' to each
> C file, that would be functionally equivalent to replacing one #include
> directive with the other, because of the include-guards. I would oppose
> actually removing '#include <svn_types.h>' from the source files,
> as a matter of style.
>
> Actually, since we have svn_hash.h, and that (I hope) includes
> apr_hash.h, and presumably files that refer to APR hash functions also
> include apr_hash.h ... we're looking at
>
> s/apr_hash.h/svn_hash.h/
>
> in every .c file. So /if/ we decide to accept the code churn, it seems
> to me this #include change makes the most sense.
Good point. +1.
There was a flaw in my argument: svn__apr_hash_index_key etc. are in svn_types.h because they are intended to be temporary names for functions that APR should eventually provide, whereas the newer 'svn_hash_gets' etc. are not.
- Julian
Re: Use svn_hash_gets
Posted by Branko Čibej <br...@wandisco.com>.
On 20.03.2013 16:16, Julian Foad wrote:
> C. Michael Pilato wrote:
>
>> Since svn_hash.h includes svn_types.h, won't this be more like replacing the
>> inclusion of the latter with the inclusion of the former?
> I'm not sure exactly what you mean, but if we do decide to leave the definitions in svn_hash.h and add '#include <svn_hash.h>' to each C file, that would be functionally equivalent to replacing one #include directive with the other, because of the include-guards. I would oppose actually removing '#include <svn_types.h>' from the source files, as a matter of style.
Actually, since we have svn_hash.h, and that (I hope) includes
apr_hash.h, and presumably files that refer to APR hash functions also
include apr_hash.h ... we're looking at
s/apr_hash.h/svn_hash.h/
in every .c file. So /if/ we decide to accept the code churn, it seems
to me this #include change makes the most sense.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Use svn_hash_gets
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> On 03/20/2013 09:45 AM, Julian Foad wrote:
>> Branko Čibej wrote:
>>> On 20.03.2013 04:47, Daniel Shahaf wrote:
>>>> See attached patch. Any objections to doing that (and similar
>>>> changes to the rest of the code)?
>>>
>>> Must we do this just before branching? I suppose it's a benign change,
>>> but it /is/ a lot of code churn.
>>
>> When I tried doing this, I found I had to include <svn_hash.h> in every
>> C file that uses them, which is fairly close to every C file in the
>> project, and that made me think it would be better if we moved the
>> definitions into <svn_types.h> which is our "global" header file.
It would not be a big deal to include svn_hash.h in the files that we're
modifying. However, these can be considered just convenience wrappers around APR hash functions, like svn__apr_hash_index_key/klen/val which are
already declared in svn_types.h so that they are globally available without
having to include svn_hash.h (since virtually all files include svn_types.h already). It makes sense to me to do the same with
svn_hash_gets/sets.
This reminds me, I wonder if we're being premature in making these new functions public.
> Since svn_hash.h includes svn_types.h, won't this be more like replacing the
> inclusion of the latter with the inclusion of the former?
I'm not sure exactly what you mean, but if we do decide to leave the definitions in svn_hash.h and add '#include <svn_hash.h>' to each C file, that would be functionally equivalent to replacing one #include directive with the other, because of the include-guards. I would oppose actually removing '#include <svn_types.h>' from the source files, as a matter of style.
>> Personally I don't mind the code churn and, for the purposes of
>> back-porting changes to 1.8.x, there is an advantage to doing it before
>> branching if we're going to do it at all (which I think we are).
>
> Speaking generally, I do mind the churn -- but this is, as was pointed out,
> a pretty benign change that requires no specialized domain knowledge to
> review. I think it's "safe enough" to go ahead and pull the trigger.
- Julian
Re: Use svn_hash_gets
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/20/2013 09:45 AM, Julian Foad wrote:
> Branko Čibej wrote:
>
>> On 20.03.2013 04:47, Daniel Shahaf wrote:
>>> See attached patch. Any objections to doing that (and similar
>>> changes to the rest of the code)?
>>
>> Must we do this just before branching? I suppose it's a benign change,
>> but it /is/ a lot of code churn.
>
> When I tried doing this, I found I had to include <svn_hash.h> in every C
> file that uses them, which is fairly close to every C file in the
> project, and that made me think it would be better if we moved the
> definitions into <svn_types.h> which is our "global" header file.
Since svn_hash.h includes svn_types.h, won't this be more like replacing the
inclusion of the latter with the inclusion of the former?
> Personally I don't mind the code churn and, for the purposes of
> back-porting changes to 1.8.x, there is an advantage to doing it before
> branching if we're going to do it at all (which I think we are).
Speaking generally, I do mind the churn -- but this is, as was pointed out,
a pretty benign change that requires no specialized domain knowledge to
review. I think it's "safe enough" to go ahead and pull the trigger.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: Use svn_hash_gets
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 20.03.2013 04:47, Daniel Shahaf wrote:
>> See attached patch. Any objections to doing that (and similar changes
>> to the rest of the code)?
>
> Must we do this just before branching? I suppose it's a benign change,
> but it /is/ a lot of code churn.
When I tried doing this, I found I had to include <svn_hash.h> in every C file that uses them, which is fairly close to every C file in the project, and that made me think it would be better if we moved the definitions into <svn_types.h> which is our "global" header file.
Personally I don't mind the code churn and, for the purposes of back-porting changes to 1.8.x, there is an advantage to doing it before branching if we're going to do it at all (which I think we are).
- Julian
Re: Use svn_hash_gets
Posted by Branko Čibej <br...@wandisco.com>.
On 20.03.2013 04:47, Daniel Shahaf wrote:
> See attached patch. Any objections to doing that (and similar changes
> to the rest of the code)?
Must we do this just before branching? I suppose it's a benign change,
but it /is/ a lot of code churn.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Use svn_hash_gets
Posted by Branko Čibej <br...@wandisco.com>.
On 20.03.2013 18:51, C. Michael Pilato wrote:
> On 03/20/2013 01:42 PM, Daniel Shahaf wrote:
>> - Do we go through with the change?
> Sure.
Ack.
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: Use svn_hash_gets
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/20/2013 01:42 PM, Daniel Shahaf wrote:
> - Do we move the macro definitions to a private header file (we can have
> svn_foo.h include that private header)?
I don't mind them being public myself. If we're going to call these
functions ourselves for the sake of convenience from a gazillion call sites,
it stands to reason that users of our public interfaces (which also generate
and consume APR hashes) would also appreciate having access to those same
conveniences.
> - Do we go through with the change?
Sure.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: Use svn_hash_gets
Posted by Daniel Shahaf <da...@apache.org>.
On Wed, Mar 20, 2013 at 05:47:11AM +0200, Daniel Shahaf wrote:
> See attached patch. Any objections to doing that (and similar changes
> to the rest of the code)?
So far I see consensus around which #include we should use, but not around
whether we should go through with the change... :-)
- Do we move the macro definitions to a private header file (we can have
svn_foo.h include that private header)?
- Do we go through with the change?
Daniel
Re: Use svn_hash_gets
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Sat, Mar 23, 2013 at 16:40:30 +0200:
> Nothing to see here; I'm just correcting the commands I used:
>
> spatch -inplace -sp_file 1 subversion/libsvn_subr/*.c
> vim
> :match Error /.\{81\}.*/
> :set aw et so=99 ai si ci sw=2 ts=2 cin cino=>2,n2,f0,{2,}0,=2,t0,+4,C1,(0,u0,*300
> :map <F3> :cn<CR>
> :map <F4> gw%
> :map <F5> f,f<Space>r<CR>gw%
> :vimgrep /svn_hash_sets.*\%81c/ subversion/libsvn_subr/*.c
> :vimgrep /svn_hash_gets.*\%81c/ subversion/libsvn_subr/*.c
> :"### (those cinoptions don't format blocks correctly, but they suffice for
> :"### the one-line task at hand here)
> ...
> :wq
And the command to add those pesky missing "svn_hash.h" includes
(apparently, some files used apr_hash_get without including apr_hash.h
directly):
vim $(make -ksj4 libsvn_repos 2>&1 | grep Error | perl -lpe 's/.*\[//; s/\..*/.c/') -c 'argdo /#include "svn_/' -c 'set aw' -c 'silent argdo norm! O#include "svn_hash.h"' -c wqa
> svn commit
>
> where...
>
>
> [[[
> % cat 1.cocci
> @@
> expression ht, key;
> @@
>
> -apr_hash_get(ht, key, APR_HASH_KEY_STRING)
> +svn_hash_gets(ht, key)
>
> @@
> expression ht, key, val;
> @@
>
> -apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
> +svn_hash_sets(ht, key, val)
>
> ]]]
>
Re: Use svn_hash_gets
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nothing to see here; I'm just correcting the commands I used:
spatch -inplace -sp_file 1 subversion/libsvn_subr/*.c
vim
:match Error /.\{81\}.*/
:set aw et so=99 ai si ci sw=2 ts=2 cin cino=>2,n2,f0,{2,}0,=2,t0,+4,C1,(0,u0,*300
:map <F3> :cn<CR>
:map <F4> gw%
:map <F5> f,f<Space>r<CR>gw%
:vimgrep /svn_hash_sets.*\%81c/ subversion/libsvn_subr/*.c
:vimgrep /svn_hash_gets.*\%81c/ subversion/libsvn_subr/*.c
:"### (those cinoptions don't format blocks correctly, but they suffice for
:"### the one-line task at hand here)
...
:wq
svn commit
where...
[[[
% cat 1.cocci
@@
expression ht, key;
@@
-apr_hash_get(ht, key, APR_HASH_KEY_STRING)
+svn_hash_gets(ht, key)
@@
expression ht, key, val;
@@
-apr_hash_set(ht, key, APR_HASH_KEY_STRING, val)
+svn_hash_sets(ht, key, val)
]]]