Kevin Pilch-Bisson <kevin@pilch-bisson.net> writes:
> I wasn't aware of that, and indeed was comtemplating using either a table
> or a hash.
Gotcha. Hmmm, if you can think of any appropriate place(s) where this
important fact -- about prop values permitting binary -- should be
stated, go ahead and add it there.
> Thinking of a doc-string along the following lines:
>
> /* Returns an apr_hash_t where the key is a char * representing the path
> from the pwd to the node on which property PROPNAME is set, and the key
> is the value of PROPNAME on that node. If TARGET is a file or RECURSE is
> false, there will be only a single element in *PROPS, with key TARGET->data.
> */
>
> Comments?
Sure. This is going to be fairly picky, so please keep in mind the
wise words Jim Blandy said to me many years ago (before totally taking
apart some Elisp code I'd written): "A sincere and thorough critique
is the highest form of flattery."
First, put the doc string in the direct, active voice (see example
below) :-). But more importantly, it needs to be much, much more
precise. For example, `pwd' is a meaningless concept in this
function, as in most of Subversion. What you really mean is that the
keys are paths relative to TARGET (I think). And what types are the
property values? It doesn't say. What does it do when the property
is absent on a given entity? It doesn't say.
Here's a possible doc string; I think it's factually correct, but it
may still have errors. Feel free to use it or just treat it as a
model, as you find appropriate:
/* Set *PROPS to a hash table whose keys are `char *' paths
* indicating the relative path from TARGET to a node on which
* property PROPNAME is set, and whose values are `svn_string_t *'
* representing the property value for PROPNAME at that path.
* Allocate *PROPS, its keys, and its values in POOL.
*
* Don't store any path, not even TARGET, if it does not have a
* property named PROPNAME.
*
* If TARGET is a file or RECURSE is false, *PROPS will have at most
* one element.
*
* If error, don't touch *PROPS, otherwise *PROPS is a hash table
* even if empty.
*/
Note how certain specifities are deliberately *not* included:
It doesn't say "Set *PROPS to an `apr_hash_t *'", because the
prototype of the function determines that itself. A doc string should
write out a C type only when the function prototype won't determine
that type. When the prototype does determine, it would be redundant
(and distracting) to use anything other than regular prose at that
point in the documentation.
It also avoids referring unnecessarily to inner members of arguments
or return values (e.g., "TARGET->data"). Again, such precision is
attention-getting, and therefore should only be used for things that
actually deserve the extra attention, or which cannot be deduced any
other way.
Hope this helps.
> I thought of having a hash of hashes (or table of hashes), similar
> to proplist, but thought this would be redundant, since we are only
> getting a single property.
Agree, yeah. Go with the shallowest data type that does the job.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:48 2006