[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: Philip Martin <philip.martin_at_wandisco.com>
Date: Tue, 14 May 2013 13:26:17 +0100

Branko Čibej <brane_at_wandisco.com> writes:

>> These warnings are "maybe" which means there will be false positives and
>> different compilers will have different results. I see a number of
>> false positives with gcc 4.7.2 and I don't think we should be adding
>> spurious initialisation to make them go away.
>> Why do people want to fix this particular instance?
> In both cases, I was not able to determine that the warnings actually
> were false positives. That depended on external circumstances outside
> the scope of the functions where the warnings were generated.

I thought it was obvious that the original code was a false positive:

  dag_node_t *here = NULL; /* The directory we're currently looking at. */
  const char *directory;

  if (flags & open_path_node_only)
      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));

  /* did the shortcut work? */
  if (here)
      path_so_far = directory;
      rest = path + strlen(directory) + 1;

The warning is on the "path_so_far = directory" line. We only get to
that line if "here" is non-NULL, that only happens if dag_node_cache_get
is called and that only happens if "directory" is assigned.

I think it is obvious the new code is also a false positive. "here"
starts as NULL, the only path that sets "here" also sets "rest". If
"here" is still NULL "rest" gets set by a different path. The result is
"rest" is always set.

> It is always better to address the warning by changing the code, than to
> pooh-pooh it away as a "maybe". I agree, however, that a simple
> initialization is often not the correct solution. Hence my attempt at
> restructuring the code so that it does not hide potential problems by
> initializing stack variables unless it's really necessary.

We fix the real positives. We might fix the false positives if there is
a reasonable change. But this warning is documented to produce false
positives and it is producing false positives in our code (I see 28
instances and all the ones I looked at are false positives). I don't
understand why this particular instance has to be fixed or proposed for
backport to 1.8.

Certified & Supported Apache Subversion Downloads:
Received on 2013-05-14 14:27:01 CEST

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.