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

On Thu, 2007-06-07 at 19:39 +0530, Kamesh Jayachandran wrote:
> Bhuvaneswaran Arumugam wrote:
> >>>
> >>>
> >> 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.
> >
> The cause of the error you see is the way macros work, i.e 'Find and
> replace'.
> <snip of your original macro with single char formal params>
> #define SVN_FS__ERR_NOT_FILE(fs, path) \
> svn_error_createf \
> (SVN_ERR_FS_NOT_FILE, 0, \
> _("'%s' is not a file in filesystem '%s'"), \
> path, fs->path)
> </snip>
> If you call SVN_FS__ERR_NOT_FILE(a, b->c) it translates to
> ....
> _("'%s' is not a file in filesystem '%s'"), \
> b->c, a->b->c)
>
>
> The following macro fixes it in a different way without compromising the
> meaning of the variables.
> #define SVN_FS__ERR_NOT_FILE(fs, path_inside_repo) \
> svn_error_createf \
> (SVN_ERR_FS_NOT_FILE, 0, \
> _("'%s' is not a file in filesystem '%s'"), \
> path_inside_repo, fs->path)
>
> I personally feel descriptive names are of great use rather than f, p.
> If some old code has 'f', 'p' they are precedants not the conventions.

To be clear, so, do you suggest to change the macro definition from:

#define SVN_FS__ERR_NOT_FILE(f, p)

to (or something similar):

#define SVN_FS__ERR_NOT_FILE(fs, path_inside_repo)

in all instances (Total: 7 macros) where we may face compile error? If
so, the naming for same parameter may vary between macros. If yes, do
you suggest me to remove the comment which explains the type of
parameters passed to each macro? The reason being, if we change the
parameter to be something meaningful and with longer name, then I think,
we need not explain the type of each argument. Hence, we can retain the
original comment which existed against the initial function definition.
Please let me know your comment.

> >>> -/* 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.
> >
>
> We use for our internal private APIs *ALL_CHARS_IN_UPPER_OF_PARAM*
> style. Here you use *USERNAME* without having any equivalent parameter.
> For a casual browser of code 'u' is not same as 'username'.

So now, I think, this change is dependent on how we go about naming the
parameter for each macro. If we are going to use a long parameter (as
per your suggestion), then I think, we can remove the type specification
all together from the comment. What do you think?

Thank you.

-- 
Regards,
Bhuvaneswaran