Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
Am 24.12.2017 um 15:54 schrieb Jeff King: On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: Yeah, I have mixed feelings on that. I think it does make the control flow less clear. At the same time, what I found was that handlers like die/ignore/warn were the thing that gave the most reduction in complexity in the callers. Would you not consider switching over to C++? With exceptions, you get the error context without cluttering the API. (Did I mention that librarification would become a breeze? Do not die in library routines: not a problem anymore, just catch the exception. die_on_error parameters? Not needed anymore. Not to mention that resource leaks would be much, MUCH simpler to treat.) I threw this email on my todo pile since I was traveling when it came, but I think it deserves a response (albeit quite late). It's been a long while since I've done any serious C++, but I did really like the RAII pattern coupled with exceptions. That said, I think it's dangerous to do it half-way, and especially to retrofit an existing code base. It introduces a whole new control-flow pattern that is invisible to the existing code, so you're going to get leaks and variables in unexpected states whenever you see an exception. I also suspect there'd be a fair bit of in converting the existing code to something that actually compiles as C++. I think I mentioned that I had a version that passed the test suite. It's not pure C++ as it required -fpermissive due to the many implicit void*-to-pointer-to-object conversions (which are disallowed in C++). And, yes, a fair bit of conversion was required on top of that. ;) So if we were starting the project from scratch and thinking about using C++ with RAII and exceptions, sure, that's something I'd entertain[1] (and maybe even Linus has softened on his opinion of C++ these days ;) ). But at this point, it doesn't seem like the tradeoff for switching is there. Fair enough. I do agree that the tradeoff is not there, in particular, when the major players are more fluent in C than in modern C++. There is just my usual rant: Why do we have look for resource leaks during review when we could have leak-free code by design? (But Dscho scored a point[*] some time ago: "For every fool-proof system invented, somebody invents a better fool.") [*] https://public-inbox.org/git/alpine.DEB.2.20.1704281334060.3480@virtualbox/
RE: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On December 24, 2017 9:54 AM, Jeff King wrote: > Subject: Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor > out rewrite_file()) > > On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > > > > Yeah, I have mixed feelings on that. I think it does make the > > > control flow less clear. At the same time, what I found was that > > > handlers like die/ignore/warn were the thing that gave the most > > > reduction in complexity in the callers. > > > > Would you not consider switching over to C++? With exceptions, you get > > the error context without cluttering the API. (Did I mention that > > librarification would become a breeze? Do not die in library routines: > > not a problem anymore, just catch the exception. die_on_error > > parameters? Not needed anymore. Not to mention that resource leaks > > would be much, MUCH simpler to treat.) > > I threw this email on my todo pile since I was traveling when it came, but I > think it deserves a response (albeit quite late). > > It's been a long while since I've done any serious C++, but I did really like > the > RAII pattern coupled with exceptions. That said, I think it's dangerous to do > it > half-way, and especially to retrofit an existing code base. It introduces a > whole new control-flow pattern that is invisible to the existing code, so > you're going to get leaks and variables in unexpected states whenever you > see an exception. > > I also suspect there'd be a fair bit of in converting the existing code to > something that actually compiles as C++. > > So if we were starting the project from scratch and thinking about using > C++ with RAII and exceptions, sure, that's something I'd entertain[1] > (and maybe even Linus has softened on his opinion of C++ these days ;) ). > But at this point, it doesn't seem like the tradeoff for switching is there. While I'm a huge fan of OO, you really need a solid justification for going there, and a good study of your target audience for Open Source C++. My comments are based on porting experience outside of Linux/Windows: 1. Conversion to C++ just to pick up exceptions is a lot like "One does not simply walk to Mordor", as Peff hinted at above. 2. Moving to C++ generally involves a **complete** redesign. While Command Patterns (and and...) may be very helpful in one level, the current git code base is very procedural in nature. 3. From a porting perspective, applications written in with C++ generally (there are exceptions) are significantly harder than C. The POSIX APIs are older and more broadly supported/emulated than what is available in C++. Once you start getting into "my favourite C++ library is...", or "version2 or version3", or smart pointers vs. scope allocation, things get pretty argumentative. It is (arguably) much easier to disable a section of code that won't function on a platform in C without having to rework an OO model, making subsequent merges pretty much impossible and the port unsustainable. That is unless the overall design really factors in platform differences right into the OO model from the beginning of incubation. 4. I really hate making these points because I am an OO "fanspert", just not when doing portable code. Even in java, which is more port-stable than C++ (arguably, but in my experience), you tend to hit platform library differences than can invalidate ports. My take is "oh my please don't go there" for the git project, for a component that has become/is becoming required core infrastructure for so many platforms. With Respect, Randall -- Brief whoami: NonStop developer since approximately UNIX(421664400)/NonStop(2112884442) -- In my real life, I talk too much.
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > > Yeah, I have mixed feelings on that. I think it does make the control > > flow less clear. At the same time, what I found was that handlers like > > die/ignore/warn were the thing that gave the most reduction in > > complexity in the callers. > > Would you not consider switching over to C++? With exceptions, you get the > error context without cluttering the API. (Did I mention that > librarification would become a breeze? Do not die in library routines: not a > problem anymore, just catch the exception. die_on_error parameters? Not > needed anymore. Not to mention that resource leaks would be much, MUCH > simpler to treat.) I threw this email on my todo pile since I was traveling when it came, but I think it deserves a response (albeit quite late). It's been a long while since I've done any serious C++, but I did really like the RAII pattern coupled with exceptions. That said, I think it's dangerous to do it half-way, and especially to retrofit an existing code base. It introduces a whole new control-flow pattern that is invisible to the existing code, so you're going to get leaks and variables in unexpected states whenever you see an exception. I also suspect there'd be a fair bit of in converting the existing code to something that actually compiles as C++. So if we were starting the project from scratch and thinking about using C++ with RAII and exceptions, sure, that's something I'd entertain[1] (and maybe even Linus has softened on his opinion of C++ these days ;) ). But at this point, it doesn't seem like the tradeoff for switching is there. -Peff [1] I'd also consider Rust, though I'm not too experienced with it myself.
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
Am 17.11.2017 um 23:33 schrieb Jeff King: On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote: On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote: Yes, I think what you've written here (and below) is quite close to the error_context patches I linked elsewhere in the thread. In other words, I think it's a sane approach. In contrast to error_context I'd like to keep all exiting behavior (die, ignore, etc.) in the hand of the caller and not use any callbacks as that makes the control flow much harder to follow. Yeah, I have mixed feelings on that. I think it does make the control flow less clear. At the same time, what I found was that handlers like die/ignore/warn were the thing that gave the most reduction in complexity in the callers. Would you not consider switching over to C++? With exceptions, you get the error context without cluttering the API. (Did I mention that librarification would become a breeze? Do not die in library routines: not a problem anymore, just catch the exception. die_on_error parameters? Not needed anymore. Not to mention that resource leaks would be much, MUCH simpler to treat.) -- Hannes
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:
> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
> > Yes, I think what you've written here (and below) is quite close to the
> > error_context patches I linked elsewhere in the thread. In other
> > words, I think it's a sane approach.
>
> In contrast to error_context I'd like to keep all exiting
> behavior (die, ignore, etc.) in the hand of the caller and not
> use any callbacks as that makes the control flow much harder to
> follow.
Yeah, I have mixed feelings on that. I think it does make the control
flow less clear. At the same time, what I found was that handlers like
die/ignore/warn were the thing that gave the most reduction in
complexity in the callers.
> Regarding the API, should it be allowed to pass NULL as error
> pointer to request no additional error handling or should the
> error functions panic on NULL? Allowing NULL makes partial
> conversions possible (e.g. for write_in_full) where old callers
> just pass NULL and check the return values and converted callers
> can use the error struct.
I think it's probably better to be explicit, and pass some "noop" error
handling struct. We'll have to be adding parameters to functions to
handle this anyway, so I don't think there's much opportunity for having
NULL as a fallback for partial conversions.
> How should translations get handled? Appending ": %s" for
> strerror(errno) might be problematic. Same goes for "outer
> message: inner message" where the helper function just inserts ":
> " between the messages. Is _("%s: %s") (with appropriate
> translator comments) enough to handle these cases?
I don't have a real opinion, not having done much translation myself. I
will say that the existing die_errno(), error_errno(), etc just use "%s:
%s", without even allowing for translation (see fmt_with_err in
usage.c). I'm sure that probably sucks for RTL languages, but I think
it would be fine to punt on it for now.
> Suggestions how to name the struct and the corresponding
> functions? My initial idea was struct error and to use error_ as
> prefix, but I'm not sure if struct error is too broad and may
> introduce conflicts with system headers. Also error_ is a little
> long and could be shorted to just err_ but I don't know if that's
> clear enough. The error_ prefix doesn't conflict with many git
> functions, but there are some in usage.c (error_errno, error,
> error_routine).
In my experiments[1] I called the types error_*, and then generally used
"err" as a local variable when necessary. Variants on that seem fine to
me, but yeah, you have to avoid conflicting with error(). We _could_
rename that, but it would be a pretty invasive patch.
> And as general question, is this approach to error handling
> something we should pursue or are there objections? If there's
> consensus that this might be a good idea I'll look into
> converting some parts of the git code (maybe refs.c) to see how
> it pans out.
I dunno. I kind of like the idea, but if the only error context is one
that adds to strbufs, I don't know that it's buying us much over the
status quo (which is passing around strbufs). It's a little more
explicit, I guess.
Other list regulars besides me seem mostly quiet on the subject.
-Peff
[1] This is the jk/error-context-wip branch of https://github.com/peff/git.
I can't remember if I mentioned that before.
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:
> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
>> Yes, I think what you've written here (and below) is quite close to the
>> error_context patches I linked elsewhere in the thread. In other
>> words, I think it's a sane approach.
>
> In contrast to error_context I'd like to keep all exiting
> behavior (die, ignore, etc.) in the hand of the caller and not
> use any callbacks as that makes the control flow much harder to
> follow.
>
>> I agree it might be nice for the error context to have a positive "there
>> was an error" flag. It's probably worth making it redundant with the
>> return code, though, so callers can use whichever style is most
>> convenient for them.
>
> Agreed.
>
> Regarding the API, should it be allowed to pass NULL as error
> pointer to request no additional error handling or should the
> error functions panic on NULL? Allowing NULL makes partial
> conversions possible (e.g. for write_in_full) where old callers
> just pass NULL and check the return values and converted callers
> can use the error struct.
>
> How should translations get handled? Appending ": %s" for
> strerror(errno) might be problematic. Same goes for "outer
> message: inner message" where the helper function just inserts ":
> " between the messages. Is _("%s: %s") (with appropriate
> translator comments) enough to handle these cases?
>
> Suggestions how to name the struct and the corresponding
> functions? My initial idea was struct error and to use error_ as
> prefix, but I'm not sure if struct error is too broad and may
> introduce conflicts with system headers. Also error_ is a little
> long and could be shorted to just err_ but I don't know if that's
> clear enough. The error_ prefix doesn't conflict with many git
> functions, but there are some in usage.c (error_errno, error,
> error_routine).
>
> And as general question, is this approach to error handling
> something we should pursue or are there objections? If there's
> consensus that this might be a good idea I'll look into
> converting some parts of the git code (maybe refs.c) to see how
> it pans out.
Any comments?
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
> Yes, I think what you've written here (and below) is quite close to the
> error_context patches I linked elsewhere in the thread. In other
> words, I think it's a sane approach.
In contrast to error_context I'd like to keep all exiting
behavior (die, ignore, etc.) in the hand of the caller and not
use any callbacks as that makes the control flow much harder to
follow.
> I agree it might be nice for the error context to have a positive "there
> was an error" flag. It's probably worth making it redundant with the
> return code, though, so callers can use whichever style is most
> convenient for them.
Agreed.
Regarding the API, should it be allowed to pass NULL as error
pointer to request no additional error handling or should the
error functions panic on NULL? Allowing NULL makes partial
conversions possible (e.g. for write_in_full) where old callers
just pass NULL and check the return values and converted callers
can use the error struct.
How should translations get handled? Appending ": %s" for
strerror(errno) might be problematic. Same goes for "outer
message: inner message" where the helper function just inserts ":
" between the messages. Is _("%s: %s") (with appropriate
translator comments) enough to handle these cases?
Suggestions how to name the struct and the corresponding
functions? My initial idea was struct error and to use error_ as
prefix, but I'm not sure if struct error is too broad and may
introduce conflicts with system headers. Also error_ is a little
long and could be shorted to just err_ but I don't know if that's
clear enough. The error_ prefix doesn't conflict with many git
functions, but there are some in usage.c (error_errno, error,
error_routine).
And as general question, is this approach to error handling
something we should pursue or are there objections? If there's
consensus that this might be a good idea I'll look into
converting some parts of the git code (maybe refs.c) to see how
it pans out.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
