Re: svn commit: r1065170 - in /subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff: diff_file.c lcs.c

Hi Daniel,

Thanks for taking a look. However ...

On Sun, Jan 30, 2011 at 4:17 AM, <danielsh_at_apache.org> wrote:
> Author: danielsh
> Date: Sun Jan 30 03:17:54 2011
> New Revision: 1065170
>
> URL: http://svn.apache.org/viewvc?rev=1065170&view=rev
> Log:
> On the 'diff-optimizations-bytes' branch:
>
> Silence compiler warnings.
>
> * subversion/libsvn_diff/lcs.c
>  (svn_diff__lcs): Add braces.
>
> * subversion/libsvn_diff/diff_file.c
>  (LOWER_7BITS_SET, BIT_7_SET, R_MASK, N_MASK, contains_eol):
>    Hide their definitions when they aren't used.
>  (find_identical_prefix):
>    Remove unused variables.
>
> Modified:
>    subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/diff_file.c
>    subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/lcs.c
>
> Modified: subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/diff_file.c
> URL: http://svn.apache.org/viewvc/subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/diff_file.c?rev=1065170&r1=1065169&r2=1065170&view=diff
> ==============================================================================
> --- subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/diff_file.c (original)
> +++ subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/diff_file.c Sun Jan 30 03:17:54 2011
> @@ -338,6 +338,7 @@ is_one_at_eof(struct file_info file[], a
>  /* Quickly determine whether there is a eol char in CHUNK.
>  * (mainly copy-n-paste from eol.c#svn_eol__find_eol_start).
>  */
> +#if SVN_UNALIGNED_ACCESS_IS_OK
>  #if APR_SIZEOF_VOIDP == 8
>  #  define LOWER_7BITS_SET 0x7f7f7f7f7f7f7f7f
>  #  define BIT_7_SET       0x8080808080808080
> @@ -349,7 +350,9 @@ is_one_at_eof(struct file_info file[], a
>  #  define R_MASK          0x0a0a0a0a
>  #  define N_MASK          0x0d0d0d0d
>  #endif
> +#endif
>
> +#if SVN_UNALIGNED_ACCESS_IS_OK
>  static svn_boolean_t contains_eol(apr_size_t chunk)
>  {
>   apr_size_t r_test = chunk ^ R_MASK;
> @@ -360,6 +363,7 @@ static svn_boolean_t contains_eol(apr_si
>
>   return (r_test & n_test & BIT_7_SET) != BIT_7_SET;
>  }
> +#endif
>
>  /* Find the prefix which is identical between all elements of the FILE array.
>  * Return the number of prefix lines in PREFIX_LINES.  REACHED_ONE_EOF will be
> @@ -377,7 +381,6 @@ find_identical_prefix(svn_boolean_t *rea
>   svn_boolean_t had_cr = FALSE;
>   svn_boolean_t is_match;
>   apr_off_t lines = 0;
> -  apr_ssize_t max_delta, delta;

... hold on a minute :-). These variable are used, although only
inside the SVN_UNALIGNED_ACCESS_IS_OK block. They are used around line
416 and following.

>   apr_size_t i;
>
>   for (i = 1, is_match = TRUE; i < file_len; i++)
> @@ -503,7 +506,7 @@ find_identical_suffix(struct file_info f
>   apr_off_t suffix_min_offset0;
>   apr_off_t min_file_size;
>   int suffix_lines_to_keep = SUFFIX_LINES_TO_KEEP;
> -  svn_boolean_t is_match, can_read, can_read_word, reached_prefix;
> +  svn_boolean_t is_match, can_read, reached_prefix;

Same here. The variable 'can_read_word' is used inside the
SVN_UNALIGNED_ACCESS_IS_OK block, at line 573 and following.

So, how should these be handled? Can we only declare those two
variables if SVN_UNALIGNED_ACCESS_IS_OK? Or maybe we can rearrange the
code a little bit to make it more clear at the same time ...

>   apr_size_t i;
>
>   memset(file_for_suffix, 0, sizeof(file_for_suffix));
>
> Modified: subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/lcs.c
> URL: http://svn.apache.org/viewvc/subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/lcs.c?rev=1065170&r1=1065169&r2=1065170&view=diff
> ==============================================================================
> --- subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/lcs.c (original)
> +++ subversion/branches/diff-optimizations-bytes/subversion/libsvn_diff/lcs.c Sun Jan 30 03:17:54 2011
> @@ -217,10 +217,12 @@ svn_diff__lcs(svn_diff__position_t *posi
>   lcs->next = NULL;
>
>   if (position_list1 == NULL || position_list2 == NULL)
> -    if (prefix_lines)
> -      return prepend_prefix_lcs(lcs, prefix_lines, pool);
> -    else
> -      return lcs;
> +    {
> +      if (prefix_lines)
> +        return prepend_prefix_lcs(lcs, prefix_lines, pool);
> +      else
> +        return lcs;
> +    }

Yes, that's much better :-).

Thanks,

-- 
Johan