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

Re: svn commit: r1469520 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Fri, 19 Apr 2013 16:42:04 +0100 (BST)

Philip Martin wrote:
> Daniel Shahaf <danielsh_at_elego.de> writes:
>>  What would a better fix be?
>
> It's hard to say which is probably why these warnings have not been
> addressed so far.  The warning is a 'maybe':
>
> ../src/subversion/libsvn_fs_fs/tree.c:929:27: warning: 'directory' may
> be used uninitialized in this function [-Wmaybe-uninitialized]
>
> so what will make it go away probably depends on various compiler
> details.  A better compiler optimiser might make the warning go away.

Agreed.

>>  The following is a first sketch, it should resolve the compiler warning
>>  but won't solve the actual semantic bug (if there is one) here.
>
> I believe it's a false positive and there is no bug, other than the
> warning, as 'directory' has to be non-NULL if 'here' is
> non-NULL.

Agreed.

I don't think Daniel's assertion made things clearer.  Here's my code rearrangement suggestion, moving the 'directory' variable into a smaller scope and avoiding it ever being uninitialized:

[[[
Index: subversion/libsvn_fs_fs/tree.c
===================================================================
--- subversion/libsvn_fs_fs/tree.c    (revision 1469858)
+++ subversion/libsvn_fs_fs/tree.c    (working copy)
@@ -910,28 +910,28 @@ open_path(parent_path_t **parent_path_p,
   apr_pool_t *iterpool = svn_pool_create(pool);

   /* callers often traverse the tree in some path-based order.  That means
      a sibling of PATH has been presently accessed.  Try to start the lookup
      directly at the parent node, if the caller did not requested the full
      parent chain. */
-  const char *directory = NULL;
   assert(svn_fs__is_canonical_abspath(path));
   if (flags & open_path_node_only)
     {
-      directory = svn_dirent_dirname(path, pool);
+      const char *directory = svn_dirent_dirname(path, pool);
+
       if (directory[1] != 0) /* root nodes are covered anyway */
         SVN_ERR(dag_node_cache_get(&here, root, directory, TRUE, pool));
+      if (here)
+        {
+          path_so_far = directory;
+          rest = path + strlen(directory) + 1;
+        }
     }

-  /* did the shortcut work? */
-  if (here)
-    {
-      path_so_far = directory;
-      rest = path + strlen(directory) + 1;
-    }
-  else
+  /* If we didn't take the shortcut ... */
+  if (! here)
     {
       /* Make a parent_path item for the root node, using its own current
          copy id.  */
       SVN_ERR(root_node(&here, root, pool));
       rest = path + 1; /* skip the leading '/', it saves in iteration */
     }

]]]

- Julian
Received on 2013-04-19 17:42:58 CEST

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