[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Kamesh Jayachandran <kamesh_at_collab.net>
Date: 2007-06-07 16:09:13 CEST

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
<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)
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.

> 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.

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'.

With regards
Kamesh Jayachandran

To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 7 16:09:10 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.