[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1849737 - in /subversion/trunk/subversion/bindings/cxx: include/svnxx.hpp include/svnxx/exception.hpp src/aprwrap/pool.hpp src/exception.cpp src/private/exception-private.hpp tests/test_exceptions.cpp

From: Branko Čibej <brane_at_apache.org>
Date: Thu, 27 Dec 2018 00:18:11 +0100

On 26.12.2018 23:44, Daniel Shahaf wrote:
> Branko Čibej wrote on Wed, 26 Dec 2018 23:37 +0100:
>> On 26.12.2018 23:21, Daniel Shahaf wrote:
>>> Branko Čibej wrote on Wed, 26 Dec 2018 22:41 +0100:
>>>> On 26.12.2018 19:50, Daniel Shahaf wrote:
>>>>> Haven't reviewed the rest of the patch, nor the mapping of
>>>>> svn_error_t::apr_err values to this hierarchy.
>>>> There is just one exception type that encapsulates all of svn_error_t,
>>>> including the apr_err bit; that's 'svn::error'. I have no intention of
>>>> going down the rabbit hole of having one exception type for each
>>>> possible apr_err value!
>>>>
>>> Yeah, that'd be way too much; but I was thinking of two things:
>>>
>>> 1. apr_err can be an errno error code, not just an APR_OS_START_USERERR
>>> code. I don't have the C++ exceptions hierarchy in mind, but I
>>> suspect that when APR_STATUS_IS_EEXIST(err->apr_err), an svn::error
>>> instance is not what people (and 'catch' blocks) will expect.
>>
>> Ah, good point. I hadn't actually thought about this side of things, as
>> clients "shouldn't" have to worry about them iff our error messages make
>> any sense. But, yes, adding such predicates would be a big help.
>>
>> They don't actually have to be part of svn::error, I'd make them
>> namespace-scope functions, e.g.:
>>
>> bool svn::error_is_eexist(const svn::error& e) noexcept;
>>
>>
>> the point being that the svn::error object serves as both an exception
>> type and a detailed error description (and that's the reason for
>> deriving svn::cancelled from svn::error).
> I don't disagree with adding such predicates, but they aren't my point.
> What I was trying to say is, doesn't the standard C++ exception
> hierarchy have some std::* exception class for, say, IO errors? In
> which case, C++ consumers might be surprised that an svn::error object
> with apr_err==EEXIST isn't caught by their 'catch' clauses for std::* IO errors?

Not so much. Or at least not as detailed as you'd think:

https://en.cppreference.com/w/cpp/error/exception

They'll expect std::ios_base::failure from one of the stream functions,
but not from Subversion. and std::filesystem::filesystem_error is
specific to the std::filesystem library, which we can't use in our API
because it's C++17 -- and even if we could use it conditionally, it
would be bad form to reuse those errors for conditions that did not
arise from use of the std::filesystem API.

I'm already changing svn::allocation_failed to be derived from
std::bad_alloc instead of std::runtime_error, but for the rest, it's
best to leave well enough alone. Users _will_ have to implement specific
error handling for the SVN++ parts of their code, or if they don't, just
rely on catch-all behaviour that may or may not lose error context.

-- Brane
Received on 2018-12-27 00:18:23 CET

This is an archived mail posted to the Subversion Dev mailing list.