Branko Čibej <br...@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:
http://www.wandisco.com/subversion/download
