C. Michael Pilato <cmpilato_at_collab.net> писал в своём письме Fri, 26 Jun
2009 23:09:34 +0400:
> Roman, I've been down the path you're on right now (briefly, some months
> ago), and suspected exactly what you seem to have found. But I kept
> getting
> so lost in wrappers and batons and casts and such that I never fully
> convinced myself of the mismatch. If this stuff is fresh in your head
> and
> you've got the time to investigate it, please PLEASE see this through to
> completion. I'll try to help where I can and as time allows -- just let
> me
> know what I can do.
>
Alright, meticulous mode on. I'll write everything down as I encounter it.
'(#)' is a shorthand for svn_wc_diff_callbacks#_t, and '::' is used in a
C++-like way to disambiguate.
There is a total of 4 generations of diff editor callbacks.
(1) has six members:
- file_changed
- file_added
- file_deleted
- dir_added
- dir_deleted
- props_changed
(2) also has six members, but their semantics are sometimes different:
- file_changed (different)
- file_added (different)
- file_deleted (different)
- dir_added (same)
- dir_deleted (same)
- dir_props_changed (new)
However, an instance of (1) is never wrapped in an instance of (2); both
of them are wrapped in an instance of (3), using different wrappers. This
is not an error, but makes things more confusing.
(3) makes the following changes:
- file_changed has a new parameter;
* the wrapper for (1) is ::file_changed;
* the wrapper for (2) is ::file_changed2;
- file_added has a new parameter;
* the wrapper for (1) is ::file_added;
* the wrapper for (2) is ::file_added2;
- file_deleted has a new parameter;
* the wrapper for (1) is ::file_deleted;
* the wrapper for (2) is ::file_deleted2;
- dir_added has a new parameter;
* the wrapper for (1) is ::dir_added;
* the wrapper for (2) is ::dir_added2;
- dir_deleted has a new parameter;
* the wrapper for (1) is ::dir_deleted;
* the wrapper for (2) is ::dir_deleted2;
- dir_props_changed has a new parameter;
* the wrapper for (1) is ::dir_props_changed;
* the wrapper for (2) is ::dir_props_changed2;
- dir_opened is new;
* the wrapper for both (1) and (2) is ::dir_opened; this is okay
because
this function doesn't use the wrapped callbacks;
- dir_closed is new;
* the wrapper for (1) and (2) is ::dir_closed; this is okay for the
same
reason;
So far, so good. An instance of (3) is wrapped in an instance of (4).
(4) makes the following changes:
- file_changed stays the same;
* the wrapper for (3) is ::file_changed2; this is WRONG, because
::file_changed2 calls (2)::file_changed, which is incompatible with
(3)::file_changed;
- file_added has new parameters;
* the wrapper for (3) is ::file_added3;
- file_deleted stays the same;
* the wrapper for (3) is ::file_deleted2; this is WRONG, because
::file_deleted2 calls (2)::file_deleted, which is incompatible with
(3)::file_deleted;
- dir_added has new parameters;
* the wrapper for (3) is ::dir_added3;
- dir_deleted stays the same;
* the wrapper for (3) is ::dir_deleted2; this is WRONG, because
::dir_deleted2 calls (2)::dir_deleted, which is incompatible with
(3)::dir_deleted;
- dir_props_changed stays the same;
* the wrapper for (3) is ::dir_props_changed2; this is WRONG, because
::dir_props_changed2 calls (2)::dir_props_changed, which is incompatible
with (3)::dir_props_changed;
- dir_opened stays the same;
* the wrapper for (3) is ::dir_opened; this is BAD, because the wrapper
must call (3)::dir_opened, but doesn't;
- dir_closed stays the same;
* the wrapper for (3) is ::dir_closed; this is BAD, because the wrapper
must call (3)::dir_closed, but doesn't;
Whew.
The items marked WRONG will cause unpredictable behaviour at run time,
ranging from nasal demons to segmentation faults. The items marked BAD
will not, but they won't behave as expected either.
The immediate fix is obviously to write new wrappers for (3) and put them
in callbacks3_wrapper. However, I'd like to offer a couple of additional
considerations:
A. The wrapping strategy is inconsistent:
(1) -> (3) -> (4)
(2) -> (3) -> (4)
(3) -> (4)
I would expect to see either:
(1) -> (2) -> (3) -> (4)
(2) -> (3) -> (4)
(3) -> (4)
or:
(1) -> (4)
(2) -> (4)
(3) -> (4)
The former is easier to extend, the latter has more runtime overhead
(probably negligible, though). The current scheme is a mix between the two
and is unobvious and error-prone. This is obviously IMHO, but still.
B. The naming scheme for wrappers is, well, poor. First, the names don't
contain an indication of "wrapperness", so the purpose isn't immediately
obvious; plus one may confuse, say, (1)::file_changed with ::file_changed.
Second, the names don't convey which generation of callbacks are they
wrapping to and from. Combined with A, this makes the names quite
unhelpful.
That's all for now.
Generally yours,
Roman.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2365979
Received on 2009-06-27 18:28:04 CEST