Bug 2550

Summary: dead assignment on object-base.cc
Product: ns-3 Reporter: natale.patriciello
Component: coreAssignee: Peter Barnes <pdbarnes>
Status: RESOLVED FIXED    
Severity: enhancement CC: ns-bugs
Priority: P5    
Version: unspecified   
Hardware: All   
OS: All   

Description natale.patriciello 2016-11-08 04:04:09 UTC 
Here the problem is a bit more interesting. In function ConstructSelf (line 80), there is an assignment to the variable "found" (line 123) that is never read, because there is a "continue" right after. Out of the loop, there is no reference to "found" ... so we basically never enter the (!found) branch, never checking the env settings and never setting the default value (even if I don't understand, if there is no attribute, how we can set the default value?!).

I can't provide the patch because I'm scared into working on the core/ subdirectory; can anyone take a look?

Thanks
Comment 1 Peter Barnes 2016-11-09 20:16:35 UTC 
(In r 12400 3dd69b81ac12, the declaration is line 115)

Stripping out the work, the skeleton of lines 115-169 is:

          bool found = false;
          if (value != 0)
            {
              // We have a matching attribute value.
              if (DoSet (info.accessor, info.checker, *value))
                {
                  found = true;
                  continue;
                }
            }              
          if (!found)
            {
              // No matching attribute value so we try to look at the env var.
              ...
                                  found = true;
              ...
            }
          if (!found)
            {
              // No matching attribute value so we try to set the default value.
            }

As you note, the "if (value !=0)" stanza doesn't need found.  As a result the first "if (!found)" stanza is guaranteed to execute, so doesn't need the test.  However, that stanza might succeed, so the second "if (!found)" test is required, as is the flag found.

I agree the declaration could move down one stanza, but it's not an unused variable.

I propose setting this bug to RESOLVED INVALID, unless Natale disagrees.
Comment 2 natale.patriciello 2016-11-10 04:39:49 UTC 
The static analyzer signals that is a dead assignment, not an unused variable. I was wrong in explaining the situation (and you corrected it clearly), but the problem still persists, we have a dead assignment.

When you assign to that variable the true value, it is never read because then there is a continue, and then the variable is initialized again with the false value. Merging this in what you said, I think that the variable can be eliminated without problems:

          if (value == 0 || !DoSet (info.accessor, info.checker, *value))
            {
              if (env ...)
                {
                }
              else 
                {
                }
            }

What do you think ?
Comment 3 Peter Barnes 2016-11-10 12:23:34 UTC 
I find that conditional too hard to read.  I'd prefer


          if (value != 0)
            {
              // We have a matching attribute value.
              if (DoSet (info.accessor, info.checker, *value))
                {
                  continue;
                }
            }              
          bool found = false;

              // No matching attribute value so we try to look at the env var.
              ...
                                  found = true;
              ...

          if (!found)
            {
              // No matching attribute value so we try to set the default value.
            }
Comment 4 natale.patriciello 2016-11-11 07:47:26 UTC 
Yes, that would be ok. Thank you!
Comment 5 Peter Barnes 2017-02-21 16:25:52 UTC 
Fixed and pushed in r12701:4a05435b2208