Julian Foad wrote on Wed, Apr 09, 2014 at 14:27:32 +0100:
> Bert Huijben wrote:
> > Julian Foad wrote:
>
> >>> In most cases it is safer to check for an error somewhere in the chain as
> >>> what is returned as the root cause (the most inner error) is not really the
> >>> error that caused the error when error chains are composed. Perhaps we should
> >>> also extend the documentation to explain this bit.
> >>
> >> You added a comment about this in the implementation in r1553266:
> >>
> >> svn_error_t *
> >> svn_error_root_cause(svn_error_t *err)
> >> {
> >> while (err)
> >> {
> >> /* I don't think we can change the behavior here, but the additional
> >> error chain doesn't define the root cause. Perhaps we should rev
> >> this function. */
> >> if (err->child /*&& err->child->apr_err != SVN_ERR_COMPOSED_ERROR*/)
> >> err = err->child;
> >> else
> >> break;
> >> }
> >>
> >> return err;
> >> }
> >>
> >> I also noticed that you made svn_error_compose_create insert the
> >> SVN_ERR_COMPOSED_ERROR marker, but not svn_error_compose. I think
> >> of those two functions as synonymous with slightly different calling
> >> conventions, but now they differ in this respect, for no apparent reason.
> >> Can you comment on that?
> >
> > I didn't add that...
> > (perhaps I might have tweaked that)
>
> Oh, sorry -- Daniel Shahaf added the separator in r1409804, and you gave it a specific error code in r1553266.
>
> > I see a lot of PROs and CONs on that behavior change...
> >
> > Yes it allows filtering for this specific case, but we have a lot of code
> > that just walks the chain... And svn_error_t is a struct&chain that is also
> > used directly by api users.
>
> Daniel, can you comment on that?
Having a "error composition without marker" API would only be useful if
we want to allow appending an error below the bottom-most error in the
linked list. Our code doesn't do that anywhere. Do third-party API
users do this? (If we don't know, I would be +0 on adding the marker to
svn_error_compose() too.)
As to changing svn_error_root_cause() to stop just before the first
SVN_ERR_COMPOSED_ERROR marker: this depends on callers to use
composition properly --- passing the "primary" error in the first
argument and the "secondary" error in the second argument. It would be
an improvement whenever callers behave this way. Even in the following
case:
svn_error_compose_create(
svn_error_compose_create(svn_error_create("foo"), svn_error_create("qux")),
svn_error_compose_create(svn_error_create("bar"), svn_error_create("baz"))
)
I think "foo" would still be the correct answer. I agree with Bert that
making this change retroactively might not be a good idea: for example,
some API users might have been passing the primary error in the second
argument, *precisely* because that convention makes the *existing*
svn_error_root_cause() DTRT for any errors generated by their own code.
(By the way, I can't help mention: in the ((foo,qux),(bar,baz)) example,
the errors' hierarchy is lossily flattened into a (foo,qux,bar,baz)
linked list. I'm not saying that it's a problem --- I don't know of any
affected codebase or use-case --- but I did want to drive-by mention it.)
Daniel
Received on 2014-04-11 10:15:27 CEST