John Peacock wrote:
> Max Bowsher wrote:
>
>> Was there any different significance between an allocated
>> svn_subst_keywords_t filled with NULL values, and a NULL
>> svn_subst_keywords_t * pointer?
>
> Well, empty strings not NULL values, but you get the idea.
Actually Max said it right: the struct could have null strings or empty
strings, and a null string meant "do not touch this keyword" whereas an empty
string meant "set this keyword's value to the empty string". A struct full of
null strings meant the same as a null struct pointer. The corresponding
semantics have been kept with the hash, where a null string in the struct
corresponds to an absent string in the hash.
> I believe the primary reason that *keywords was ever set to NULL was to
> call some of the functions when you explicitly didn't want to deal with
> the keywords.
Yes. That's why it's a bit of a special case: the concept is not that a
keywords hash can be null, but that some functions take an _optional_ keywords
hash, and thus accept NULL as the alternative to a keywords hash. This is only
useful in functions that have a use other than processing keywords, but it so
happens that three out of the four keywords functions have an additional use
(processing eol-style):
svn_subst_keywords_differ2(a,b) - mandatory
svn_subst_translate_stream3(keywords) - optional
svn_subst_copy_and_translate3(keywords) - optional
svn_subst_translate_cstring2(keywords) - optional
And keywords_differ had already been written to accept NULL, for no particular
reason AFAIK (it was just easy to do and perhaps convenient for some callers),
so we have the curious situation of the whole keywords-hash-consuming API
accepting NULL.
The keywords-hash-producing API, the single function
svn_subst_build_keywords2(), never produces NULL, nor should it.
> Look at the code, it appears that the only time you would get an empty
> hash is if svn:keywords was set but contained no legal keyword values. I
OK.
> think this is even more evidence that svn_subst_build_keywords2() should
> never actively return an empty hash and all code should be adjusted to
> correspond.
No, that's a non-sequitor. It doesn't imply anything.
> To make Julian more comfortable, perhaps we could create a typedef to
> alias apr_hash_t, and define that type as having two possible values:
>
> a) NULL
> b) a hash containing at least one key/value pair
Leaving aside whether (a) should be NULL or an empty hash:
There's some merit in that idea, but firstly we don't seem to have a convention
of defining type aliases for such things, and secondly if it is documented
sufficiently at every place in the pulic API where it exists, which I think it
is now, then that's sufficient. In other words, we've defined an API as a set
of functions that take a certain type of data. We could also define the data
container as a separate public type, but we don't need to if the required form
is sufficiently easy to understand and get right. I think it now is
sufficiently easy. I'm not saying a defined type wouldn't be better - cleaner
- but just that the way it is now is good enough.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 23 00:40:19 2005