Re: [PATCH] libsvn-fs-util: Convert error related functions asmacros

On Mon, 2007-06-04 at 16:15 +0530, Kamesh Jayachandran wrote:
> Hi Bhuvan,
>
> > Index: subversion/include/private/svn_fs_util.h
> > ===================================================================
> > --- subversion/include/private/svn_fs_util.h (revision 25279)
> > +++ 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 */
> > @@ -41,33 +42,62 @@
> >
> > /* SVN_ERR_FS_NOT_MUTABLE: the caller attempted to change a node
> > outside of a transaction. */
> >
> May be you can have the above comment as
>
> /* Attempted to change a node outside of a transaction. */
> *
> *
>
> Your comment should try to explain the type of args passed to Macro.
>
> **
>
> > -svn_error_t *svn_fs__err_not_mutable(svn_fs_t *fs, svn_revnum_t rev,
> > - const char *path);
> > +#define SVN_FS__ERR_NOT_MUTABLE(f, r, p) \
> > + svn_error_createf \
> > + (SVN_ERR_FS_NOT_MUTABLE, 0, \
> > + _("File is not mutable: filesystem '%s', revision %ld, path '%s'"), \
> > + f->path, r, p)
> >
> > /* SVN_ERR_FS_NOT_DIRECTORY: PATH does not refer to a directory in FS. */
> >
>
> I think you need not explain this macro as it is self evident. Your
> comment should try to explain the type of args passed to Macro.
>
> > -svn_error_t *svn_fs__err_not_directory(svn_fs_t *fs, const char *path);
> > +#define SVN_FS__ERR_NOT_DIRECTORY(f, p) \
> > + svn_error_createf \
> > + (SVN_ERR_FS_NOT_DIRECTORY, 0, \
> > + _("'%s' is not a directory in filesystem '%s'"), \
> > + p, f->path)
> >
> > /* SVN_ERR_FS_NOT_FILE: PATH does not refer to a file in FS. */
> >
> I think you need not explain this macro as it is self evident. Your
> comment should try to explain the type of args passed to Macro.
>
> > -svn_error_t *svn_fs__err_not_file(svn_fs_t *fs, const char *path);
> > +#define SVN_FS__ERR_NOT_FILE(f, p) \
> > + svn_error_createf \
> > + (SVN_ERR_FS_NOT_FILE, 0, \
> > + _("'%s' is not a file in filesystem '%s'"), \
> > + p, f->path)
> >
> > /* SVN_ERR_FS_PATH_ALREADY_LOCKED: a path is already locked. */
> >
>
> I think you need not explain this macro as it is self evident. Your
> comment should try to explain the type of args passed to Macro.
>
> > -svn_error_t *svn_fs__err_path_already_locked(svn_fs_t *fs,
> > - svn_lock_t *lock);
> > +#define SVN_FS__ERR_PATH_ALREADY_LOCKED(f, l) \
> > + svn_error_createf \
> > + (SVN_ERR_FS_PATH_ALREADY_LOCKED, 0, \
> > + _("Path '%s' is already locked by user '%s' in filesystem '%s'"), \
> > + l->path, l->owner, f->path)
> >
> > /* SVN_ERR_FS_NO_SUCH_LOCK: there is no lock on PATH in FS. */
> >
> I think you need not explain this macro as it is self evident. Your
> comment should try to explain the type of args passed to Macro.
> > -svn_error_t *svn_fs__err_no_such_lock(svn_fs_t *fs, const char *path);
> > +#define SVN_FS__ERR_NO_SUCH_LOCK(f, p) \
> > + svn_error_createf \
> > + (SVN_ERR_FS_NO_SUCH_LOCK, 0, \
> > + _("No lock on path '%s' in filesystem '%s'"), \
> > + p, f->path)
> >
> > /* SVN_ERR_FS_LOCK_EXPIRED: TOKEN's lock in FS has been auto-expired. */
> >
> I think you need not explain this macro as it is self evident. Your
> comment should try to explain the type of args passed to Macro.
> > -svn_error_t *svn_fs__err_lock_expired(svn_fs_t *fs, const char *token);
> > +#define SVN_FS__ERR_LOCK_EXPIRED(f, t) \
> > + svn_error_createf \
> > + (SVN_ERR_FS_LOCK_EXPIRED, 0, \
> > + _("Lock has expired: lock-token '%s' in filesystem '%s'"), \
> > + t, f->path)
> >
> > /* SVN_ERR_FS_NO_USER: FS does not have a user associated with it. */
> >
> I think you need not explain this macro as it is self evident. Your
> comment should try to explain the type of args passed to Macro.
> > -svn_error_t *svn_fs__err_no_user(svn_fs_t *fs);
> > +#define SVN_FS__ERR_NO_USER(f) \
> >
> Retain the original formal variable names.
> > + svn_error_createf \
> > + (SVN_ERR_FS_NO_USER, 0, \
> > + _("No username is currently associated with filesystem '%s'"), \
> > + f->path)
> >
> > /* SVN_ERR_FS_LOCK_OWNER_MISMATCH: trying to use a lock whose LOCK_OWNER
> > doesn't match the USERNAME associated with FS. */
> >
> May be you can change this to
>
> /* SVN_FS__ERR_LOCK_OWNER_MISMATCH: trying to use a lock whose LOCK_OWNER
> doesn't match the USERNAME associated with FS. */
>
> Document the type of formal arguments in your comment.

Kamesh, thank you for reviewing the patch. I've incorporated all your
suggestion and attached the revised patch for review.

-- 
Regards,
Bhuvaneswaran