Re: [PATCH] Get-Locations TNG Rev. 26 - Using the New Specification

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

+ 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;

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.

+ /* TODO: should it be svn_ra_svn_write_number()
+ * instead? */

As previously discussed, no, it shouldn't be, so this comment can go.

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

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

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.

+ 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);

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

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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 29 16:32:04 2004