Ok, I feel like we are all on the same page now, right?
As reasonably possible throw library specific exceptions from all public
methods.
No exception should directly extend any non-library exception, like std
exceptions.
All Exceptions shall not use multi-inheritance.
All exceptions should reasonably extend less specific but logically
relevant base exceptions.
All exceptions shall extend a root library exception, Exception, either
directly or indirectly through inheritance.
The root library exception shall extend std::exception.
A few quick examples:
namespace apache {
namespace geode {
namespace client {
class Exception : public std::exception {...};
class IllegalArgumentException : public Exception {...};
class TransactionException : public Exception {...};
class RollbackException : public TransactionException {...};
// NO - class IllegalArgumentException : public Exception, public
std::invalid_argument {...};
// NO - class IllegalArgumentException : public std::invalid_argument {...};
// NO - class IllegalArgumentException : public Exception, public
TransactionException {...};
}
}
}
Additionally, investigate using Boost Stack Track library for providing
stack context in exceptions, otherwise dump the current stack tracing
feature that is incomplete and very platform specific.
Does anyone have a different understanding of the consensus?
On Thu, Sep 14, 2017 at 2:24 PM Michael William Dodge <mdo...@pivotal.io>
wrote:
> +1 for avoiding multiple inheritance
>
> > On 14 Sep, 2017, at 14:23, Ernest Burghardt <eburgha...@pivotal.io>
> wrote:
> >
> > Sounds like the proposal currently stands to avoid the DiamondOfDeath or
> > TriangleOfLove that multiple inheritance brings us....
> > and just have the base Exception class inherit std::exception and extend
> > Exception class as appropriate within the library.
> >
> > +1 and thanks for the code examples, very illistrative!
> >
> > +1 to Boost stack trace as well...
> >
> >
> > On Thu, Sep 14, 2017 at 10:10 AM, Jacob Barrett <jbarr...@pivotal.io>
> wrote:
> >
> >> The problem stems from the fact that none of the std exceptions
> virtually
> >> inherit from std::exception so you end up in the inheritance triangle of
> >> love. I say we avoid the multiple inheritance issues with exceptions by
> >> avoiding multiple inheritance altogether in exceptions. See this
> example.
> >> http://coliru.stacked-crooked.com/a/2e32e7777b021e85
> >>
> >> Basically all of our exceptions extend our Exception class which extends
> >> std::exception. None extend any of the other std exceptions, like
> >> bad_alloc, etc. The downside is you can't catch any std exception other
> >> than std::exception but the upside is that this actually works. There
> were
> >> also very few exceptions that actually could extend any std exceptions
> >> beyond std::exception.
> >>
> >> -Jake
> >>
> >>
> >> On Wed, Sep 13, 2017 at 10:52 PM Jacob Barrett <jbarr...@pivotal.io>
> >> wrote:
> >>
> >>> Let me see if I can considerate the consensus here:
> >>>
> >>> As reasonably possible throw library specific exceptions from all
> public
> >>> methods.
> >>>
> >>> As reasonably possible those exceptions should extend C++11 standard
> >>> library exceptions.
> >>>
> >>> All exceptions should extend less specific but logically relevant base
> >>> exceptions.
> >>>
> >>> A few quick examples:
> >>> namespace apache {
> >>> namespace geode {
> >>> namespace client {
> >>>
> >>> class Exception : public std::exception {...};
> >>>
> >>> class IllegalArgumentException : public Exception, public
> >>> std::invalid_argument {...};
> >>>
> >>> class TransactionException : public Exception {...};
> >>>
> >>> class RollbackException : public TransactionException {...};
> >>>
> >>> }
> >>> }
> >>> }
> >>>
> >>> Additionally, investigate using Boost Stack Track library for providing
> >>> stack context in exceptions, otherwise dump the current stack tracing
> >>> feature that is incomplete and very platform specific.
> >>>
> >>> Does anyone have a different understanding of the consensus?
> >>>
> >>>
> >>> I found some problems with this model. The IllegalArgumentException
> would
> >>> not be caught in a catch (const std::exception& e) statement do to
> >>> multiple inheritance. Another nasty is std::exception doesn't have a
> >>> constructor to populate the what() value. Some examples of this problem
> >>> can be seen here:
> >>> http://coliru.stacked-crooked.com/a/5b6e34c7ea020fc8
> >>>
> >>>
> >>> -Jake
> >>>
> >>>
> >>>
> >>>
> >>> On Wed, Sep 13, 2017 at 4:37 PM Mark Hanson <mhan...@pivotal.io>
> wrote:
> >>>
> >>>> I think that it would be best to abide by using the std::exception as
> >> the
> >>>> base. I think it brings with it all the language support and
> >> flexibility.
> >>>> There are a few penalties obviously to using std::exception as a base,
> >> but
> >>>> I think they are sufficiently offset by using a standard. As far as
> the
> >>>> number of exceptions, I believe in the philosophy of using as few as
> >>>> possible and using inheritance to drive specificity. The benefit is
> that
> >>>> the code can be as simple or as complex as it can be without
> unnecessary
> >>>> overhead e.g error => network error => tcp error. So, I may just care
> >> there
> >>>> is a network error and the being able to tell if something derives
> from
> >>>> network error is perfect.
> >>>>
> >>>> Thanks,
> >>>> Mark
> >>>>
> >>>>
> >>>>> On Sep 8, 2017, at 1:35 PM, Ernest Burghardt <eburgha...@pivotal.io>
> >>>> wrote:
> >>>>>
> >>>>> if we continue the merging of Jake <> Sarge comments we might find
> >> that
> >>>>> std::exception(s) is sufficient if the many name exceptions that
> >>>> pertain to
> >>>>> the Library are all handled in a similar manner and only have unique
> >>>> names
> >>>>> for human readability, but not a functional consideration...
> >>>>>
> >>>>> EB
> >>>>>
> >>>>> On Fri, Sep 8, 2017 at 8:52 AM, Michael William Dodge <
> >>>> mdo...@pivotal.io>
> >>>>> wrote:
> >>>>>
> >>>>>> I subscribe to Josh Gray's philosophy of only having another
> >> exception
> >>>>>> class if there is something different to be done when it's caught.
> >> For
> >>>>>> example, if the caller would do the exact same thing for
> >>>>>> NoPermissionsException and DiskFullException, just use an
> IOException
> >>>> and
> >>>>>> be done with it. I also subscribe to the philosophy that a library
> >>>> have its
> >>>>>> own exception hierarchy (possibly with a single class), which I
> think
> >>>>>> meshes with Jake's "exceptions exiting a library to be reasonably
> >>>> limited
> >>>>>> to exceptions generated by and relating to that library".
> >>>>>>
> >>>>>> Sarge
> >>>>>>
> >>>>>>> On 8 Sep, 2017, at 07:19, Jacob Barrett <jbarr...@pivotal.io>
> >> wrote:
> >>>>>>>
> >>>>>>> Sorry for jumping on this thread so late. This is an important
> issue
> >>>> we
> >>>>>>> need to address.
> >>>>>>>
> >>>>>>> On Thu, Aug 17, 2017 at 11:57 AM David Kimura <dkim...@pivotal.io>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>>> Using exceptions seems contrary to the Google C++ Style Guide we
> >>>>>> adopted,
> >>>>>>>> which states: *"do not use C++ exceptions"* [3
> >>>>>>>> <https://google.github.io/styleguide/cppguide.html#Exceptions>].
> >>>> Here
> >>>>>> is
> >>>>>>>> a
> >>>>>>>> link [4 <https://www.youtube.com/watch?v=NOCElcMcFik#t=30m29s>]
> >> to a
> >>>>>>>> cppcon
> >>>>>>>> talk defending their decision. Does it make sense to enforce this
> >>>> rule
> >>>>>> on
> >>>>>>>> our code base?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I don't agree with this approach, as I always tend towards
> >>>>>>> progress/modernization, but it was their choice to remain
> consistent
> >>>>>> across
> >>>>>>> their code base. I would say if we made the same decision solely on
> >>>>>>> consistency then we would have our own exceptions derived from a
> >> base
> >>>>>>> exception class. This is consistent with our Java and .NET as well
> >> as
> >>>>>>> current C++ clients.
> >>>>>>>
> >>>>>>>
> >>>>>>>> If we decide to knowingly ignore this rule, would we want to
> >>>> continue to
> >>>>>>>> propagate custom exceptions or switch to standard exceptions? At
> >> the
> >>>>>> very
> >>>>>>>> least, it seems to me that our custom exceptions should derive
> from
> >>>>>> their
> >>>>>>>> most closely matching standard exception counterparts. Thoughts?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I agree with your recommendation of using our exceptions but extend
> >>>> the
> >>>>>>> most appropriate C++11 exceptions where applicable. I think it is
> >> good
> >>>>>> form
> >>>>>>> for exceptions exiting a library to be reasonably limited to
> >>>> exceptions
> >>>>>>> generated by and relating to that library.
> >>>>>>>
> >>>>>>> Our current Exception class also brings with it stack tracing,
> >>>> although
> >>>>>>> poorly supported on some platforms and disabled by default. Boost
> >> just
> >>>>>>> recently graduated a stack track library that is supported on many
> >>>>>>> compilers and platforms. It may be worth integrating into our
> >>>> exceptions
> >>>>>> as
> >>>>>>> a replacement for the current stack tracing code.
> >>>>>>>
> >>>>>>> -Jake
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
>
>
