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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Daniel Shahaf-2
[hidden email] wrote on Wed, 26 Dec 2018 04:28 +0000:
> Reimplement exceptions in SVN++.
>
> Instead of extracting error messages from the svn_error_t when the exception
> is created, keep the svn_error_t embedded in the exception and use its contents
> only when needed.

> +++ subversion/trunk/subversion/bindings/cxx/include/svnxx/exception.hpp Wed Dec 26 04:28:25 2018
> @@ -31,6 +31,46 @@
> +/**
> + * @defgroup svnxx_exceptions Exceptions
> + *
> + * Exceptions in SVN++
> + * ===================
> + *
> + * SVN++ uses exceptions for the following purposes:
> + * @li Reporting memory allocation failure; where Subversion's
> + *     default hehaviour is to abort when an allocation from an APR
> + *     pool fails, SVN++ throws an exception instead.
> + * @li Reporting errors; Subversion's error messages are wrapped in
> + *     exceptions.
> + * @li Reporting cancelled operations; an operation that was
> + *     cancelled from user code will report this by throwing a
> + *     specific exception type.
> + * @li Terminating iteration; user-level callbacks may throw a
> + *     specific exception type to cancel an ongoing operation that
> + *     is generating the callback messages. Other exceptions from
> + *     user-level callbacks will be propagated back to the calling
> + *     application.

This part is an excellent overview; if it's not already in the C doxygen
docs or in HACKING, it should be (mutatis mutandis).

> + * Exception Hierarchy
> + * -------------------
> + *
> + * All SVN++ exceptions are ultimately derived from @c std:::exception.
> + *
> + * * <em>std::exception</em>
> + *   + <em>std::runtime_error</em>
> + *     - apache::subversion::svnxx::allocation_failed\n
> + *       Thrown when memory cannot be allocated from an APR pool
> + *   + apache::subversion::svnxx::error\n
> + *     Thrown when an operation failed (see @ref svn_error_t)
> + *     - apache::subversion::svnxx::canceled\n
> + *       Thrown when an operation was canceled
> + *   + apache::subversion::svnxx::cancel\n
> + *     Thrown by user callbacks to terminate iteration

I think the last two names here are too similar; it'll be way too easy
to mis-tab-complete them for each other.  Suggest to rename the second
one to stop_iteration to match the name of SVN_ERR_STOP_ITERATION.

Haven't reviewed the rest of the patch, nor the mapping of
svn_error_t::apr_err values to this hierarchy.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
On 26.12.2018 19:50, Daniel Shahaf wrote:

> [hidden email] wrote on Wed, 26 Dec 2018 04:28 +0000:
>> Reimplement exceptions in SVN++.
>>
>> Instead of extracting error messages from the svn_error_t when the exception
>> is created, keep the svn_error_t embedded in the exception and use its contents
>> only when needed.
>> +++ subversion/trunk/subversion/bindings/cxx/include/svnxx/exception.hpp Wed Dec 26 04:28:25 2018
>> @@ -31,6 +31,46 @@
>> +/**
>> + * @defgroup svnxx_exceptions Exceptions
>> + *
>> + * Exceptions in SVN++
>> + * ===================
>> + *
>> + * SVN++ uses exceptions for the following purposes:
>> + * @li Reporting memory allocation failure; where Subversion's
>> + *     default hehaviour is to abort when an allocation from an APR
>> + *     pool fails, SVN++ throws an exception instead.
>> + * @li Reporting errors; Subversion's error messages are wrapped in
>> + *     exceptions.
>> + * @li Reporting cancelled operations; an operation that was
>> + *     cancelled from user code will report this by throwing a
>> + *     specific exception type.
>> + * @li Terminating iteration; user-level callbacks may throw a
>> + *     specific exception type to cancel an ongoing operation that
>> + *     is generating the callback messages. Other exceptions from
>> + *     user-level callbacks will be propagated back to the calling
>> + *     application.
> This part is an excellent overview; if it's not already in the C doxygen
> docs or in HACKING, it should be (mutatis mutandis).
>
>> + * Exception Hierarchy
>> + * -------------------
>> + *
>> + * All SVN++ exceptions are ultimately derived from @c std:::exception.
>> + *
>> + * * <em>std::exception</em>
>> + *   + <em>std::runtime_error</em>
>> + *     - apache::subversion::svnxx::allocation_failed\n
>> + *       Thrown when memory cannot be allocated from an APR pool
>> + *   + apache::subversion::svnxx::error\n
>> + *     Thrown when an operation failed (see @ref svn_error_t)
>> + *     - apache::subversion::svnxx::canceled\n
>> + *       Thrown when an operation was canceled
>> + *   + apache::subversion::svnxx::cancel\n
>> + *     Thrown by user callbacks to terminate iteration
> I think the last two names here are too similar; it'll be way too easy
> to mis-tab-complete them for each other.  Suggest to rename the second
> one to stop_iteration to match the name of SVN_ERR_STOP_ITERATION.

Actually users can't create objects of type 'svn::canceled' — or at
least not without a lot of effort, as there are no public constructors.
I would hope a smart IDE would notice, or else at least a smart user
will. :)

But yes, I hear you. stop_iteration is a good suggestion.

> 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!

TL;DR:

class error : public std::exception,
              protected std::shared_ptr<svn_error_t>
{
public:
  // ...
  const char* what();   // "best" error message (override)
  int code();           // converted apr_status_t
  const char* name();   // symbolic error name, when available
  // ...
};

Reply | Threaded
Open this post in threaded view
|

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

Daniel Shahaf-2
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.

2. SVN_ERROR_IN_CATEGORY(...).  I'll say no more about it since we use
   it very rarely even on the C side of things.

> TL;DR:
>
> class error : public std::exception,
>               protected std::shared_ptr<svn_error_t>
> {
> public:
>   // ...
>   const char* what();   // "best" error message (override)
>   int code();           // converted apr_status_t
>   const char* name();   // symbolic error name, when available
>   // ...
> };

Thanks for this.

HTH,

Daniel
Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
In reply to this post by Branko Čibej
On 26.12.2018 22:41, Branko Čibej wrote:

> On 26.12.2018 19:50, Daniel Shahaf wrote:
>> [hidden email] wrote on Wed, 26 Dec 2018 04:28 +0000:
>>> Reimplement exceptions in SVN++.
>>>
>>> Instead of extracting error messages from the svn_error_t when the exception
>>> is created, keep the svn_error_t embedded in the exception and use its contents
>>> only when needed.
>>> +++ subversion/trunk/subversion/bindings/cxx/include/svnxx/exception.hpp Wed Dec 26 04:28:25 2018
>>> @@ -31,6 +31,46 @@
>>> +/**
>>> + * @defgroup svnxx_exceptions Exceptions
>>> + *
>>> + * Exceptions in SVN++
>>> + * ===================
>>> + *
>>> + * SVN++ uses exceptions for the following purposes:
>>> + * @li Reporting memory allocation failure; where Subversion's
>>> + *     default hehaviour is to abort when an allocation from an APR
>>> + *     pool fails, SVN++ throws an exception instead.
>>> + * @li Reporting errors; Subversion's error messages are wrapped in
>>> + *     exceptions.
>>> + * @li Reporting cancelled operations; an operation that was
>>> + *     cancelled from user code will report this by throwing a
>>> + *     specific exception type.
>>> + * @li Terminating iteration; user-level callbacks may throw a
>>> + *     specific exception type to cancel an ongoing operation that
>>> + *     is generating the callback messages. Other exceptions from
>>> + *     user-level callbacks will be propagated back to the calling
>>> + *     application.
>> This part is an excellent overview; if it's not already in the C doxygen
>> docs or in HACKING, it should be (mutatis mutandis).
>>
>>> + * Exception Hierarchy
>>> + * -------------------
>>> + *
>>> + * All SVN++ exceptions are ultimately derived from @c std:::exception.
>>> + *
>>> + * * <em>std::exception</em>
>>> + *   + <em>std::runtime_error</em>
>>> + *     - apache::subversion::svnxx::allocation_failed\n
>>> + *       Thrown when memory cannot be allocated from an APR pool
>>> + *   + apache::subversion::svnxx::error\n
>>> + *     Thrown when an operation failed (see @ref svn_error_t)
>>> + *     - apache::subversion::svnxx::canceled\n
>>> + *       Thrown when an operation was canceled
>>> + *   + apache::subversion::svnxx::cancel\n
>>> + *     Thrown by user callbacks to terminate iteration
>> I think the last two names here are too similar; it'll be way too easy
>> to mis-tab-complete them for each other.  Suggest to rename the second
>> one to stop_iteration to match the name of SVN_ERR_STOP_ITERATION.
> Actually users can't create objects of type 'svn::canceled' — or at
> least not without a lot of effort, as there are no public constructors.
> I would hope a smart IDE would notice, or else at least a smart user
> will. :)
>
> But yes, I hear you. stop_iteration is a good suggestion.


Out of interest ... where is SVN_ERR_STOP_ITERATION defined? I can't
find it on trunk.

-- Brane

Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
In reply to this post by Daniel Shahaf-2
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).

-- Brane


Reply | Threaded
Open this post in threaded view
|

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

Daniel Shahaf-2
In reply to this post by Branko Čibej
Branko Čibej wrote on Wed, 26 Dec 2018 23:28 +0100:

> On 26.12.2018 22:41, Branko Čibej wrote:
> > On 26.12.2018 19:50, Daniel Shahaf wrote:
> >> [hidden email] wrote on Wed, 26 Dec 2018 04:28 +0000:
> >>> + *     - apache::subversion::svnxx::canceled\n
> >>> + *       Thrown when an operation was canceled
> >>> + *   + apache::subversion::svnxx::cancel\n
> >>> + *     Thrown by user callbacks to terminate iteration
> >> I think the last two names here are too similar; it'll be way too easy
> >> to mis-tab-complete them for each other.  Suggest to rename the second
> >> one to stop_iteration to match the name of SVN_ERR_STOP_ITERATION.
> > Actually users can't create objects of type 'svn::canceled' — or at
> > least not without a lot of effort, as there are no public constructors.
> > I would hope a smart IDE would notice, or else at least a smart user
> > will. :)
> >
> > But yes, I hear you. stop_iteration is a good suggestion.
>
>
> Out of interest ... where is SVN_ERR_STOP_ITERATION defined? I can't
> find it on trunk.

Sorry, SVN_ERR_ITER_BREAK.  StopIteration is a Python exception class.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

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

Daniel Shahaf-2
In reply to this post by Branko Čibej
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?
Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
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

Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
In reply to this post by Daniel Shahaf-2
On 26.12.2018 23:41, Daniel Shahaf wrote:

> Branko Čibej wrote on Wed, 26 Dec 2018 23:28 +0100:
>> On 26.12.2018 22:41, Branko Čibej wrote:
>>> On 26.12.2018 19:50, Daniel Shahaf wrote:
>>>> [hidden email] wrote on Wed, 26 Dec 2018 04:28 +0000:
>>>>> + *     - apache::subversion::svnxx::canceled\n
>>>>> + *       Thrown when an operation was canceled
>>>>> + *   + apache::subversion::svnxx::cancel\n
>>>>> + *     Thrown by user callbacks to terminate iteration
>>>> I think the last two names here are too similar; it'll be way too easy
>>>> to mis-tab-complete them for each other.  Suggest to rename the second
>>>> one to stop_iteration to match the name of SVN_ERR_STOP_ITERATION.
>>> Actually users can't create objects of type 'svn::canceled' — or at
>>> least not without a lot of effort, as there are no public constructors.
>>> I would hope a smart IDE would notice, or else at least a smart user
>>> will. :)
>>>
>>> But yes, I hear you. stop_iteration is a good suggestion.
>>
>> Out of interest ... where is SVN_ERR_STOP_ITERATION defined? I can't
>> find it on trunk.
> Sorry, SVN_ERR_ITER_BREAK.  StopIteration is a Python exception class.

Right. I admit I also keep thinking that "Of course we have
SVN_ERR_STOP_ITERATION." :)

-- Brane

Reply | Threaded
Open this post in threaded view
|

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

James McCoy-3
In reply to this post by Branko Čibej
On Thu, Dec 27, 2018 at 12:18:11AM +0100, Branko Čibej wrote:

> 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

Don't forget the system error stuff introduced in c++11:

https://en.cppreference.com/w/cpp/header/system_error

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Reply | Threaded
Open this post in threaded view
|

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

Branko Čibej
On 27.12.2018 01:18, James McCoy wrote:

> On Thu, Dec 27, 2018 at 12:18:11AM +0100, Branko Čibej wrote:
>> 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
> Don't forget the system error stuff introduced in c++11:
>
> https://en.cppreference.com/w/cpp/header/system_error

Good point, we should be able to map that.

http://subversion.apache.org/issue/4799

-- Brane