Hi kou,
On 9/16/07, Kouhei Sutou <kou@cozmixng.org> wrote:
> Hi Joe,
>
> 2007/9/16, Joe Swatosh <joe.swatosh@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
Received on Sun Sep 16 18:11:04 2007