On Sep 08, 2007, at 09:14:12, David Glasser wrote:
>> + /* The file 'yours' points to edit_baton's cached
>> empty file. Since
>> + * it was just moved, set it to empty so that later
>> get_empty_file()
>> + * calls don't get it wrong with the cache, i.e.
>> create another
>> + * empty file again. */
>> + sprintf((char *)yours, ""); /* awful cast */
>
> That strikes me as a strange use of sprintf; how about just
> *yours = '\0';
> ?
More importantly, it seems to me that “awful cast” is an understatement.
It ought to be no less than a #warning.
It's not obvious from the diff, since it doesn't cover the function
signature, but “yours” is declared as const char *:
static svn_error_t *
merge_file_added([etc.],
const char *mine,
const char *older,
----> const char *yours,
svn_revnum_t rev1,
svn_revnum_t rev2,
[etc.])
This is dangerous. It assumes that the storage behind “yours” will never
really be const. This *may* be true now (and I wouldn't know), but even
if it is, can you guarantee that the storage will never be const in
the future?
I apologize if I'm speaking out of turn, given that I'm not an svn
developer, have never submitted a patch for svn, and don't even know the
svn codebase very well. But the don't-write-to-const rule is universal,
so I felt I had to point this out, lest this potential bug become a real
bug at some point in the future.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 10 01:59:21 2007