On Thursday 29 April 2004 17:31, Greg Hudson wrote:
> I reviewed only the ra_svn parts; however, while looking for them, I
> happened to notice the following:
>
> +static int compare_revnum_t (const void *p_a, const void *p_b)
>
> We use the _t suffix for type names, not function names. Call this
> "compare_revnums".
>
Fixed.
> + if (a < b)
> + {
> + return 1;
> + }
> + else if (a > b)
> + {
> + return (-1);
> + }
> + else
> + {
> + return 0;
> + }
>
> Twelve lines where one would do:
>
> return (a < b) ? 1 : (a > b) ? -1 : 0;
>
OK.
> Moving onto the ra_svn client:
>
> + /* Transmit the parameters over the line
> + * */
>
> One of my style complaints was using two lines for a one-line comment,
> and not punctuating comments. I'd prefer in general not to have the
> extra "* */" on a line by itself at the end of comments in the ra_svn
> code (client and server). Please write comments like this as:
>
> /* Transmit the parameters over the line. */
>
> Better yet, omit the "over the line" since it's imprecise (what's a
> "line"?) and doesn't add anything.
>
OK. Done.
> + /* TODO: should it be svn_ra_svn_write_number()
> + * instead? */
>
> As previously discussed, no, it shouldn't be, so this comment can go.
>
Done.
> + if (item->kind == SVN_RA_SVN_WORD &&
> + (strcmp(item->u.word, "done") == 0))
>
> if (item->kind == SVN_RA_SVN_WORD && strcmp(item->u.word, "done") ==
> 0)
>
Done.
> + apr_hash_set(fs_locations, apr_pmemdup(pool,
> + (const
> void*)&past_revision,
> +
> sizeof(past_revision)),
> + sizeof(past_revision), (const void*)ret_path);
>
> You do not need to cast parameters to const void *. This appears to
> apply to many other places in the code. Also, always put a space
> between the type name and the * when using a pointer type.
>
Done. I may have misapplied what you said earlier about the representation of
pointers to different type not necessarily having the same representation.
> In serve.c:
>
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + "Get-locations location revisions entry
> not a revision");
>
> Do not exceed 80 columns. You can split a C string into two parts
> ("foobar" -> "foo" "bar"), allowing you to reflow long lines like that.
> Alternatively, you could come up with a shorter error string.
>
Hmmm... done. In any case, I'm not so sure how you expect lines not to exceed
80 columns with the aligning the parameters to the function open parens style
guideline.
> + err = svn_fs_trace_node_locations(b->fs,
> + &fs_locations,
> + abs_path,
> + peg_revision,
> + location_revisions,
> + pool);
>
> err = svn_fs_trace_node_locations(b->fs, &fs_locations, abs_path,
> peg_revision, location_revisions,
> pool);
>
Done.
> + for(iter = apr_hash_first(pool, fs_locations) ;
> + iter ;
> + iter = iter = apr_hash_next(iter))
>
> for (iter = apr_hash_first(pool, fs_locations); iter;
> iter = apr_hash_next(iter))
>
> (Note particularly the correction of "iter = iter = ...", but also the
> addition of the space after "for", the elimination of the space before
> the semicolons, and the collapsing onto two lines.)
>
So "for"'s do have a space after them?
Fine, fixed.
> + {
> + const void * key;
> + apr_ssize_t key_len; /* Unused */
> + void * value;
>
> Please move these declarations to the top of the function. (Another
> ra_svn-specific style point I failed to mention before, but this seems
> to be the only place it comes into question. Also, no need to declare a
> dummy key_len variable; just pass NULL as the third parameter of
> apr_hash_this. And don't put a space after the "*" when declaring a
> pointer.
>
Done.
Regards,
Shlomi Fish
---------------------------------------------------------------------
Shlomi Fish shlomif@iglu.org.il
Homepage: http://shlomif.il.eu.org/
Quidquid latine dictum sit, altum viditur.
[Whatever is said in Latin sounds profound.]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 29 19:18:10 2004