Thanks for the review comment Kamesh. Please see my responses inline.
On Thu, 2007-06-07 at 18:37 +0530, Kamesh Jayachandran wrote:
> Thanks Bhuvan.
>
> > Index: subversion/include/private/svn_fs_util.h
> > ===================================================================
> > --- subversion/include/private/svn_fs_util.h (revision 25291)
> > +++ subversion/include/private/svn_fs_util.h (working copy)
> > @@ -20,6 +20,7 @@
> > #ifndef SVN_FS_UTIL_H
> > #define SVN_FS_UTIL_H
> >
> > +#include "svn_private_config.h"
> > #ifdef __cplusplus
> > extern "C" {
> > #endif /* __cplusplus */
> > @@ -39,35 +40,66 @@
> > error if this is not the case. */
> > svn_error_t *svn_fs__check_fs(svn_fs_t *fs);
> >
> > -/* SVN_ERR_FS_NOT_MUTABLE: the caller attempted to change a node
> > - outside of a transaction. */
> > -svn_error_t *svn_fs__err_not_mutable(svn_fs_t *fs, svn_revnum_t rev,
> > - const char *path);
> > +/* SVN_FS__ERR_NOT_MUTABLE: attempted to change a node outside of a
> > + transaction. F is of type "svn_fs_t *", R is of type
> > + "svn_rev_num_t *", P is of type "const char *". */
> >
>
> Can you retain original namings like 'fs', 'rev', 'path' instead of 'f',
> 'r' and 'p'.? Same with other macros too.
In most of cases if we use original naming convention, we face a
compiler error as indicated in this link [1]. That is one of reason for
me to change the variable names on most of occasion, assuming it does
not harm and I also noted that this convention is followed elsewhere in
the code base.
[1] http://paste.lisp.org/display/42134
> > -/* SVN_ERR_FS_NO_USER: FS does not have a user associated with it. */
> > -svn_error_t *svn_fs__err_no_user(svn_fs_t *fs);
> >
> Explain type of 'fs' like you did for other macros.
In this case, I did not explain the type of "fs" argument, as it is
obvious! In addition, elsewhere in code base, I do not see any occasion
where we explain the parameter for a macro when it is obvious.
> > -/* SVN_ERR_FS_LOCK_OWNER_MISMATCH: trying to use a lock whose LOCK_OWNER
> > - doesn't match the USERNAME associated with FS. */
> > -svn_error_t *svn_fs__err_lock_owner_mismatch(svn_fs_t *fs,
> > - const char *username,
> > - const char *lock_owner);
> > +/* SVN_FS__ERR_LOCK_OWNER_MISMATCH: trying to use a lock whose
> > + LOCK_OWNER doesn't match the USERNAME associated with FS.
> > + F is of type "svn fs_t *", U is of type "const char *",
> > + O is of type "const char *". */
> >
> Use the original function argument names. Here you have 'u', but doc
> says 'USERNAME'.
The USERNAME explains the context for the parameter though the type for
this argument is explained underneath.
Let me know what you think. Thank you very much.
--
Regards,
Bhuvaneswaran
Received on Thu Jun 7 15:27:02 2007