Agreed.
I had some reticence against this approach due to concerns around
cross-DLL linking of data variables, but Bert thinks we should be
okay. Potentially a few build tweaks to support the data bits, but
then we'll be good.
I'm going to go ahead and commit the basic bits, and then we can grow
usage from there.
Thanks!
-g
On Thu, Aug 27, 2009 at 12:31, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> 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=2394223
Received on 2009-09-13 13:24:39 CEST