Blair Zajac wrote:
> hwright@tigris.org wrote:
>> Author: hwright
>> Date: Thu Apr 12 09:02:49 2007
>> New Revision: 24557
>>
>> Log:
>> JavaHL: Add implicit type conversion to the C++ Path class, allowing
>> objects of this class to be directly used wherever a const char * is
>> expected.
>>
>> Suggested by: dlr
>
> I don't think this is such a good idea. There's too much magic going on
> here :)
Insert Arther C. Clark quote here...
> Is the point here just to save some typing?
Well, I think that the point is to be able to use Path just as we would
a const char * when calling functions that are part of our C api. I
like those semantics, because it makes the C function feels like it can
handle the C++ objects, and just Do The Right Thing with them.
Maybe that's too idealistic, and maybe we should just go back to using
explicit conversion functions. Whatever the outcome, it would be good
to document the consensus somewhere so that we can refer to it later.
> I would rather have the conversion be explicit. The c_str() method also
> makes it clear on the ownership of the memory. For somebody who isn't
> too familiar with the JavaHL bindings, I would much rather see c_str()
> in the code and immediately know the ownership issues than wonder what's
> going on. Also, with this change, there's no documentation on who owns
> the memory after the implicit conversion, one has to check the code.
Then let's document it. The C++ bit of the JavaHL bindings could use
quite a bit of additional documentation as it is.
> Also, note that the C++ standard committee deliberately didn't choose to
> have this conversion for std::string, I don't think we should use it for
> Path either.
In response to r24558, Blair Zajac wrote:
> hwright@tigris.org wrote:
>> Author: hwright
>> Date: Thu Apr 12 10:09:55 2007
>> New Revision: 24558
>>
>> Log:
>> JavaHL: Add implicit type conversion to the C++ Revision class, allowing
>> objects of this class to be directly used wherever a
>> svn_opt_revision_t * is
>> expected. Also add a couple of helper functions to return the
>> revision number
>> and kind of the wrapped revision.
>
> Hi Hyrum,
>
> Let's hold off on these implicit type conversions until we reach a
> consensus on them. I don't think they are a good design decision.
>
> As others have written:
>
> "The upshot is that while conversion operators are handy, they
> compromise type safety and disarm the compiler of its type-safety
> checks. For this reason, library designers either forgo conversion
> operators at the cost of inconveniencing users (the lack of a const char
> * conversion operator in std::string is a classic example of this), or
> they resort to embarrassingly contrived constructs to support this
> notion without its dangerous consequences."
>
> http://www.informit.com/guides/content.asp?g=cplusplus&seqNum=297&rl=1
Interesting. The standards committee must have included the implicit
conversion operator capabilities for /some/ reason, though. If the only
reason *not* to use it is that there's "too much magic" going on, I'm
not sure that I completely buy that. I'm just concerned that we're
throwing the baby out with the bathwater in an attempt to make things
foolproof.
>> Modified: trunk/subversion/bindings/javahl/native/CopySources.cpp
>> URL:
>>
http://svn.collab.net/viewvc/svn/trunk/subversion/bindings/javahl/native/CopySources.cpp?pathrev=24558&r1=24557&r2=24558
>>
>>
==============================================================================
>>
>> --- trunk/subversion/bindings/javahl/native/CopySources.cpp (original)
>> +++ trunk/subversion/bindings/javahl/native/CopySources.cpp Thu Apr
>> 12 10:09:55 2007
>> @@ -129,7 +129,7 @@
>> Revision rev(jrev);
>> src->revision = (const svn_opt_revision_t *)
>> apr_palloc(pool, sizeof(*src->revision));
>> - memcpy((void *) src->revision, rev.revision(),
>> + memcpy((void *) src->revision, (const svn_opt_revision_t
>> *) rev,
>
> This syntax is definitely not any cleaner. I would much rather see
> rev.revision() than (const svn_opt_revision_t *) rev.
True, but I think that more an artifact of what is going on here. Using
C++ constructs, couldn't we just do something like:
Revision rev(jrev);
src->revision = rev.copy(pool);
and let the object handle the copy? This would eliminate both the
contrived cast, as well as the ugly syntax associated with the memcpy().
-Hyrum
PS - I'm not trying to be confrontational here, and I hope this hasn't
come across that way. Just trying to understand the issues.
Received on Thu Apr 12 20:49:31 2007