On Wed, Apr 18, 2012 at 8:18 AM, Julian Foad <julianf...@btopenworld.com> wrote: > Hyrum K Wright wrote: > >> On Wed, Apr 18, 2012 at 2:15 AM, Julian Foad wrote: >>> Blair Zajac wrote: >>>> In case of an illegal svn_checksum_kind_t being passed to >>>> svn_checksum__from_digest(), I want to change it from >>>> >>>> svn_checksum_t * >>>> svn_checksum__from_digest(const unsigned char *digest, >>>> svn_checksum_kind_t kind, >>>> apr_pool_t *result_pool); >>>> >>>> to >>>> >>>> svn_error_t * >>>> svn_checksum__from_digest(svn_checksum_t **checksum, >>>> const unsigned char *digest, >>>> svn_checksum_kind_t kind, >>>> apr_pool_t *result_pool); >>> >>> Wouldn't a better API be: >>> >>> svn_checksum_t * >>> svn_checksum__from_digest_md5(digest, result_pool); >>> svn_checksum_t * >>> svn_checksum__from_digest_sha1(digest, result_pool); >>> >>> since all current callers want just one specific kind (apart from internal >>> calls from the implementations of svn_checksum_dup and >> svn_checksum_empty_checksum)? >> >> No. This would be the only case where a specific checksum kind is >> embedded in the API name rather than as a parameter. The latter makes >> extending the API easier in the future, and localizes the 'switch' >> statement to that method. > > It *would* make extending the API easier and localize the 'switch' *if* there > were parameterized calls... but there aren't any. So there is no need for > any 'switch' statement, in practice. Look at the use cases. There are 14 > callers. The 12 callers outside of checksum.c all want an already-known > kind, not a parameterized kind. The two internal callers each handle any > kind, but one of them (_empty_checksum) already contains a switch on the > kind. That leaves only one caller, the implementation of _dup(), where a > parameterized API could be beneficial, but such implementation is of course > free to use local static functions and other short-cuts.
There aren't any such callers *now*. That does not preclude the addition of them in the future. > We could certainly keep the parameterized API for cases when it would be > useful, and if it were public that would be the right thing to do, but it's > private. > >> What would be the benefit of separate APIs? > > Consistency with the style of related APIs is good, granted. Simplicity > of APIs is good too. Simplicity prefers avoiding possible error > conditions by design, rather than coding to detect and handle errors. > Omitting the 'kind' input and the 'error' output (where the only possible > error is 'you passed an invalid kind') makes a much simpler API. > > In response to consistency: after writing the above I've just realized why > this function really is different from the others. Most of the checksum > functions that take a 'kind' input will create whatever kind you like, > independently of their other params. On the other hand, *this* function > takes two parameters that are required to agree with each other: the 'digest' > parameter has to point to an MD5 digest iff 'kind'=md5, and to a SHA1 digest > iff kind=sha1. It's not like 'const char *digest' is the universal standard > external representation of an arbitrary checksum kind; although it would be > possible for any checksum kind to have an external repr that can be addressed > as 'char *', the next kind we add might prefer to use a different external > representation such as 'struct foo'. So it's not wierd to have one > 'checksum' constructor function per checksum kind. That would be the normal > pattern for constructor functions. That argument makes sense. -Hyrum >>> Notice that svn_checksum_empty_checksum() returns NULL if the kind is not >> recognized, while svn_checksum_dup() does no such check and would pass the >> unknown kind into ...from_digest(). >>> >>> Therefore it appears that _dup() is the only caller that could have been >> passing a bad kind -- at least in current trunk code; it may be different in >> whatever version you're running. And so, to fix the crash, you will still >> need to decide how _dup() should handle a bogus checksum struct. You could >> have >> _dup() return NULL, but unless the callers check for null that would just >> defer >> the crash. It would be a little better to call >> SVN_ERR_MALFUNCTION_NO_RETURN(), >> to give the client program a little more awareness of the abnormal >> termination. >>> >>> >>>> This is to prevent a core dump we've observed with our RPC server. >>>> >>>> The function is in a public .h file but shown as private. Do I need to >> add >>>> svn_checksum__from_digest2() or can I just change it? >>> >>> We're allowed to just change it, as I understand it. >>> >>> (The closest thing I can find in 'hacking' is >> <http://subversion.apache.org/docs/community-guide/conventions.html#interface-visibility>. >> But that doesn't seem to mention the nuances of ABI vs. API and the issue >> of, IIRC, all symbols in a shared library getting into a Windows DLL ABI >> even if >> they're supposed to be private, or whatever exactly that issue was.) >> >> Nobody should be calling this function anyway, so ABI shouldn't be an >> issue, IIUC. >> >> -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
