You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kouhei Sutou <ko...@cozmixng.org> on 2007/09/16 01:04:28 UTC
Re: svn commit: r26614 - in trunk/subversion/bindings/swig:
include ruby/libsvn_swig_ruby ruby/svn
Hi Joe,
In <20...@svn2.sjc.collab.net>
"svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn" on Sat, 15 Sep 2007 12:46:05 -0700,
joeswatosh@tigris.org wrote:
> Author: joeswatosh
> Date: Sat Sep 15 12:46:05 2007
> New Revision: 26614
>
> Log:
> Follow on to r26471 which merged copy-on-updates branch to trunk. This merely
> gets the existing tests passing, it doesn't actually add new capabilities to
> the bindings. Some TODOs to remember to implement the functionality later.
>
> This is a slightly different implementation than has been used in the past for
> callbacks. Instead of exporting the callback implementation directly I
> created a "getter" function so if the svn_wc_get_file_t interface changes we
> will get a compiler error.
I think it's better that we get a compiler error.
If we use a "getter" function and the svn_wc_get_file_t
interface is changed, we will not get correct behavior on
runtime. I think it's easy to detect incorrect behavior if
a compiler reports a error rather than tests report a
runtime error.
Thanks,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Joe Swatosh <jo...@gmail.com>.
Hi kou
On 9/16/07, Kouhei Sutou <ko...@cozmixng.org> wrote:
> Hi Joe,
>
> 2007/9/17, Joe Swatosh <jo...@gmail.com>:
> > > > > > Author: joeswatosh
> > > > > > Date: Sat Sep 15 12:46:05 2007
> > > > > > New Revision: 26614
> > > > > >
> If VC compiler doesn't report this, _getter() approach is good for me too.
> If VC compiler reports this, it seems that _getter() approach is needless...
>
I retried and VC does report the mismatch. I just missed it in the
mess of ouput that VC emits. I removed the getter function and the
svn_swig_rb_fetch_func in r26655 and just set the fetch_func and
fetch_baton to NULL in the typemap.
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi Joe,
2007/9/17, Joe Swatosh <jo...@gmail.com>:
> > > > > Author: joeswatosh
> > > > > Date: Sat Sep 15 12:46:05 2007
> > > > > New Revision: 26614
> > > > >
> > > > I think it's better that we get a compiler error.
> > > >
> > > > If we use a "getter" function and the svn_wc_get_file_t
> > > > interface is changed, we will not get correct behavior on
> > > > runtime. I think it's easy to detect incorrect behavior if
> > > > a compiler reports a error rather than tests report a
> > > > runtime error.
> > >
> > > I know these aren't supposed to change after a release, but we had one
> > > change that was under development and it was hard for me to find. I
> > > think this way is better while things might change. After release it
> > > doesn't matter so much.
> >
> > Can you find the interface change easily with this change?
> > If it's true this change is OK.
> >
> > But It seems that it's not true. Because you didn't write tests
> > for this change. I'm afraid that the interface change is ignored
> > and we forget to follow the interface change when release.
> >
> I misunderstood. I thought you liked the new "_getter()" idiom because it
> would cause the compiler to let us know earlier. Here is my experience:
Ah, no. I don't like it.
> To establish a base, this is what a normal build looks like for me today:
...
> If I make this change (which causes our callback to no longer match the
> svn_wc_conflict_resolver_func_t defined interface):
...
> My build results look like:
...
> IOW, no different from the build without the error.
I think this should be reported in subversion/bindings/swig/ruby/svn_wc.c.
Because the type mismatch is caused when we assign
svn_swig_rb_conflict_resolver_func() to svn_wc_conflict_resolver_func_t
variable. On my environment, I'm using gcc, the following warning is
reported:
subversion/bindings/swig/ruby/svn_wc.c: In function
'_wrap_svn_wc_get_update_editor3':
subversion/bindings/swig/ruby/svn_wc.c:14535: warning: assignment
from incompatible pointer type
And subversion/bindings/swig/ruby/svn_wc.c:14535 is the following:
arg11 = svn_swig_rb_conflict_resolver_func;
And here is arg11 decralation:
svn_wc_conflict_resolver_func_t arg11 = (svn_wc_conflict_resolver_func_t) 0 ;
If VC compiler doesn't report this, _getter() approach is good for me too.
If VC compiler reports this, it seems that _getter() approach is needless...
Thanks,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Joe Swatosh <jo...@gmail.com>.
Hi kou,
On 9/16/07, Kouhei Sutou <ko...@cozmixng.org> wrote:
> Hi Joe,
>
> 2007/9/16, Joe Swatosh <jo...@gmail.com>:
>
> > > > Author: joeswatosh
> > > > Date: Sat Sep 15 12:46:05 2007
> > > > New Revision: 26614
> > > >
> > > I think it's better that we get a compiler error.
> > >
> > > If we use a "getter" function and the svn_wc_get_file_t
> > > interface is changed, we will not get correct behavior on
> > > runtime. I think it's easy to detect incorrect behavior if
> > > a compiler reports a error rather than tests report a
> > > runtime error.
> >
> > I know these aren't supposed to change after a release, but we had one
> > change that was under development and it was hard for me to find. I
> > think this way is better while things might change. After release it
> > doesn't matter so much.
>
> Can you find the interface change easily with this change?
> If it's true this change is OK.
>
> But It seems that it's not true. Because you didn't write tests
> for this change. I'm afraid that the interface change is ignored
> and we forget to follow the interface change when release.
>
Right. That's why I left the TODO in there, and why I didn't modify the
public interface of Svn::Wc::AdmAccess#update_editor, but just passed a nil as
the fetch_func argument to the Wc.get_update_editor3 call. Since I didn't
write the test, I tried to do the minimum possible to maintain existing
function. My thinking is that at least we can use the old function of
Svn::Wc::AdmAccess#update_editor without raising due to calling
Wc.get_update_editor3 with the wrong number of arguments for now. When we
write a test for the new ability then we can expose it by adding the find_func
parameter to Svn::Wc::AdmAccess#update_editor.
As I reread your post, I'm afraid I'm misunderstanding again.
I misunderstood. I thought you liked the new "_getter()" idiom because it
would cause the compiler to let us know earlier. Here is my experience:
To establish a base, this is what a normal build looks like for me today:
D:\SVN\src-trunk>msdev D:\SVN\src-trunk\subversion_msvc.dsw /USEENV
/MAKE "libsvn_swig_ruby - Win32 release" | findstr /v
/c:"D:\SVN\httpd-2.0.59"
--------------------Configuration: libsvn_swig_ruby - Win32
Release--------------------
Compiling...
swigutil_rb.c
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1142)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE_WITH_CONVENIENCE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1142)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1143)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE'
Linking...
Creating library
..\..\..\Release\subversion\bindings\swig\ruby\libsvn_swig_ruby/libsvn_swig_ruby-1.lib
and object ..\..\..\Release\subversion\bindings\swig\ruby\libsvn_swig_ruby/libsvn_swig_ruby-1.exp
libsvn_swig_ruby-1.dll - 0 error(s), 3 warning(s)
******************************************************************************
If I make this change (which causes our callback to no longer match the
svn_wc_conflict_resolver_func_t defined interface):
Index: subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h
===================================================================
--- subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h
(revision 26624)
+++ subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.h
(working copy)
@@ -222,6 +222,7 @@
svn_wc_conflict_result_t *result,
const svn_wc_conflict_description_t *description,
void *baton,
+ void *another_argument,
apr_pool_t *pool);
SVN_RB_SWIG_SWIGUTIL_EXPORT
Index: subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
===================================================================
--- subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
(revision 26624)
+++ subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
(working copy)
@@ -2188,6 +2188,7 @@
svn_swig_rb_conflict_resolver_func(svn_wc_conflict_result_t *result,
const
svn_wc_conflict_description_t *description,
void *baton,
+ void *another_argument,
apr_pool_t *pool)
{
svn_error_t *err = SVN_NO_ERROR;
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
My build results look like:
D:\SVN\src-trunk>msdev D:\SVN\src-trunk\subversion_msvc.dsw /USEENV
/MAKE "libsvn_swig_ruby - Win32 release" | findstr /v
/c:"D:\SVN\httpd-2.0.59"
--------------------Configuration: libsvn_swig_ruby - Win32
Release--------------------
Compiling...
swigutil_rb.c
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1142)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE_WITH_CONVENIENCE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1142)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1143)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE'
Linking...
Creating library
..\..\..\Release\subversion\bindings\swig\ruby\libsvn_swig_ruby/libsvn_swig_ruby-1.lib
and object ..\..\..\Release\subversion\bindings\swig\ruby\libsvn_swig_ruby/libsvn_swig_ruby-1.exp
libsvn_swig_ruby-1.dll - 0 error(s), 3 warning(s)
IOW, no different from the build without the error.
******************************************************************************
On the other hand, if I make this change (which causes our call back to no
longer match the svn_wc_get_file_t defined interface):
Index: subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
===================================================================
--- subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
(revision 26624)
+++ subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c
(working copy)
@@ -2221,6 +2221,7 @@
svn_stream_t *stream,
svn_revnum_t *fetched_rev,
apr_hash_t **props,
+ void* another_argument,
apr_pool_t *pool)
{
svn_error_t *err = SVN_NO_ERROR;
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
My build looks like:
D:\SVN\src-trunk>msdev D:\SVN\src-trunk\subversion_msvc.dsw /USEENV
/MAKE "libsvn_swig_ruby - Win32 release" | findstr /v
/c:"D:\SVN\httpd-2.0.59"
--------------------Configuration: libsvn_swig_ruby - Win32
Release--------------------
Compiling...
swigutil_rb.c
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1142)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE_WITH_CONVENIENCE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1142)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(1143)
: warning C4003: not enough actual parameters for
macro'DEFINE_DUP_BASE'
D:\SVN\src-trunk\subversion\bindings\swig\ruby\libsvn_swig_ruby\swigutil_rb.c(2244)
: warning C4113: 'struct svn_error_t *(__cdecl *)(void *
,const char *,long ,struct svn_stream_t *,long *,struct apr_hash_t **
,void *,struct apr_pool_t *)' differs in parameter lists from 'struct
svn_error_t *(__cdecl *)(void *,const char *,long ,struct svn_stream_t
*,long *,struct apr_hash_t ** ,struct apr_pool_t *)'
Linking...
Creating library
..\..\..\Release\subversion\bindings\swig\ruby\libsvn_swig_ruby/libsvn_swig_ruby-1.lib
and object ..\..\..\Release\subve
rsion\bindings\swig\ruby\libsvn_swig_ruby/libsvn_swig_ruby-1.exp
libsvn_swig_ruby-1.dll - 0 error(s), 4 warning(s)
******************************************************************************
So with the VC compiler I at least get a warning if our callback doesn't match
the expected interface which is all I was trying to achieve. Now obviously,
this is only one compiler. If yours has different behavior that will help you
catch these kinds of errors in some other way, I have no problem using the
existing idiom.
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi Joe,
2007/9/16, Joe Swatosh <jo...@gmail.com>:
> > > Author: joeswatosh
> > > Date: Sat Sep 15 12:46:05 2007
> > > New Revision: 26614
> > >
>
>
> >
> > I think it's better that we get a compiler error.
> >
> > If we use a "getter" function and the svn_wc_get_file_t
> > interface is changed, we will not get correct behavior on
> > runtime. I think it's easy to detect incorrect behavior if
> > a compiler reports a error rather than tests report a
> > runtime error.
>
> I know these aren't supposed to change after a release, but we had one
> change that was under development and it was hard for me to find. I
> think this way is better while things might change. After release it
> doesn't matter so much.
Can you find the interface change easily with this change?
If it's true this change is OK.
But It seems that it's not true. Because you didn't write tests
for this change. I'm afraid that the interface change is ignored
and we forget to follow the interface change when release.
Thanks,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn
Posted by Joe Swatosh <jo...@gmail.com>.
Hi kou,
On 9/15/07, Kouhei Sutou <ko...@cozmixng.org> wrote:
> Hi Joe,
>
> In <20...@svn2.sjc.collab.net>
> "svn commit: r26614 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn" on Sat, 15 Sep 2007 12:46:05 -0700,
> joeswatosh@tigris.org wrote:
>
> > Author: joeswatosh
> > Date: Sat Sep 15 12:46:05 2007
> > New Revision: 26614
> >
>
> I think it's better that we get a compiler error.
>
> If we use a "getter" function and the svn_wc_get_file_t
> interface is changed, we will not get correct behavior on
> runtime. I think it's easy to detect incorrect behavior if
> a compiler reports a error rather than tests report a
> runtime error.
I know these aren't supposed to change after a release, but we had one
change that was under development and it was hard for me to find. I
think this way is better while things might change. After release it
doesn't matter so much.
--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org