On Wed, 2009-08-26 at 14:17 -0400, Greg Stein wrote:
> Hey all,
>
> In my last shift of code from wc_db.c to upgrade.c, I had to duplicate
> one of the functions that converts between values and string constants
> (a "word"). We have a bunch of these functions all over the place that
> convert to/from values. And when my plane landed a couple hours ago, I
> see that Daniel committed Yet Another One in r38944.
>
> I've attached two files for review/comment before I really start
> filling it out, and committing the thing.
>
> This implementation is very simplistic. If we find performance
> problems, then we can create some hand-coded or scripted solutions to
> improve the speed.
>
> The code is targeted for libsvn_subr, but it "reaches higher" into
> headers from higher-level systems to get enumerated values. There are
> no *link* loops, however, so I believe this is okay.
>
> Comments, please!
What I like:
* Consistent way of declaring and converting them.
* This method is already in use in libsvn_wc/tree_conflicts.c (I
thought I recognized it :-)
* The semantics that the callers want are provideed - especially the
choice of how an unrecognized word should be handled.
What could be better:
* I'd like it better if the mapping tables were scoped to the module
that needs them, rather than centralized. For example, several mappings
are only needed at the client level, and some others are only needed in
"libsvn_wc/tree-conflicts.c", even though all of the enums on which they
are based are needed more widely.
There may be a perceived convenience in having "standard" string
representations centralized in one utility module, such as
"none/file/dir" for svn_node_kind_t. But thinking more strictly, it is
always a higher level's responsibility to choose the right set of
strings for the task in hand, e.g. the choice between "directory" and
"dir" depending on the UI style.
* Needs to provide for localization of some of the sets of strings.
e.g. a variant where strings are declared as N_("deleted") and gettext
is called in the _to_word() conversion function.
* Implementation could be simplified by publicizing pointers to the
maps themselves, rather than an enum that identifies the maps and has to
be looked up in a separate step. (This would also help reduce the
centralization of knowledge.)
Other comments:
* Sometimes might want to simplify the point of call with a dedicated
set of function names per data type: e.g. "#define
svn_wc_conflict_to_word(value) svn_token_to_word(conflict_map, value)".
Your approach doesn't prevent this. There are already some functions
like this, e.g. svn_cl__operation_str_xml().
* Some data types have multiple sets of strings. e.g.
svn_wc_conflict_action_t has one map for human-readable output, another
for (not localized) XML output, another for the WC "entries" file data
format. These happen to all use the same words at the moment:
_("deleted")/"deleted"/"deleted" - but they could just as well be all
different: _("deleted")/"del"/"D". Your approach doesn't prevent
multiple mapping tables at all, but this is an additional argument for
decentralizing the tables.
Take a look at my version, attached.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387938
Received on 2009-08-27 18:31:34 CEST