On Thu, Dec 6, 2012 at 2:40 PM, Shivani Poddar
<shivani.poddar92_at_gmail.com> wrote:
> Log Message:
> subversion/bindings/swig/include
> (svn_checksum_t.swg) : Generated a typemap.
> (svn_checksum_kind_t,svn_checksum_create,svn_checksum_clear): typemapped
> these functions
>
> Suggested by: Stefan Sperling <stsp_at_elego.de>
> Ben Reser <breser_at_fornix.brain.org>
> Daniel Shahaf <~danielsh_at_apache>
In this case I'd say that the three people you're providing
attribution to here are all committers. Their attribution can be
shortened to just their usernames. Which are: stsp, breser, danielsh.
> I hope this PATCH is incorporated and i am able to contribute substantially
> to the subversion community.
In addition to the things Daniel already brought up, some comments on
your patch.
1) The most critical piece of feedback I can give you here is that you
need to ensure that your patch is functional and achieves the stated
end. Which means at a minimum you need to write some Python code that
uses the interfaces you have exposed with the bindings. The Python
bindings have a test suite, I'd advise that you add code that
exercises the interface. This means that not only have you
demonstrated that it works to yourself but we are able to ensure that
it stays working.
2) You've created a new checksum module. Why? As I tried to explain
on IRC we break the modules up by the Subversion libraries. There is
a somewhat out of date NOTES document in the subversion/bindings/swig
directory that explains this. You can find it online here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/bindings/swig/NOTES
In my opinion svn_checksum.h should be added to _core which is used
for things in libsvn_subr and that don't fit anywhere else.
3) You have two "%typemap(in) svn_checksum_t *" definitions. I don't
understand what you need this for. You should only need one typemap
for a given type and SWIG method ('in' is the SWIG method).
4) The typemaps shouldn't need to call svn_checksum_create() or
svn_checksum_clear(). Typemaps just convert types between C and the
Binding Language. It's best to think of the typemap as code that is
injected into the wrapper that SWIG is building, where in the wrapper
it is injected depends on the method of the typemap. Refer to the
SWIG documentation I pointed you at on IRC.
In this case maintaining the struct and providing accessor functions
to it would be the right way to go about this. Simply converting to a
string won't be very helpful since you'll be throwing away the kind of
checksum (something Daniel has already mentioned).
5) As I'd mentioned on IRC the major things you have to write a
typemap for is argouts and callbacks (there are some other minor
cases). There are svn_checksum_t's that are used as output arguments
so you do need to implement the argout. However, the struct and the
enum should just be handled automatically for you by SWIG in the other
cases. You may have to do some work to enable accessor functions for
the struct members, I'm not sure what's needed on the Python side for
that. Which leads me to ...
6) Taking a look at the checksum related functions and types I see
that the symbols aren't showing up to SWIG. So someone needs to pull
in those symbols into a SWIG module. That should be a simple matter
of adding the auto-generated .swg file for the header file to the
module.
The following comments are nitpicky comments about your patch. Given
the above you need to majorly rework the patch in order for it to be
functional. However, I still mention these things since we do try to
maintain code quality and a functional patch with these sorts of
issues will likely receive these sorts of comments.
1) You start a comment on the same line at the end of the checksum
module definition. I would put this on a separate line.
2) svn_checksum_clear is mistyped as avn_checksum_clear.
3) Your patch file has just the filename, instead of the full path.
We typically run svn diff from the top level of the wc so as to make
your patch easier to apply.
4) The log message is not in the proper format per the community
guide: http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
I'll be around this weekend so if you have any questions, please feel
free to ask. I'll do my best to help you get your patch in shape so
that it can be committed.
Received on 2012-12-07 21:22:08 CET