Closed Bug 1372386 Opened 7 years ago Closed 7 years ago

nsSprocketLayout.cpp: Unused variable "last"

Categories

(Core :: XUL, enhancement)

Product:
Core
Component:
XUL
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1286390])

Attachments

(1 file, 1 obsolete file)

Bug 1372386 - Remove trailing whitespace
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
      No description provided.
Assignee: nobody → sledru
Comment on attachment 8876905 [details]
Bug 1372386 - nsSprocketLayout.cpp: Remove useless declaration

https://reviewboard.mozilla.org/r/148222/#review152616

::: layout/xul/nsSprocketLayout.cpp:799
(Diff revision 1)
> -        if (prefWidth > biggestPrefWidth) 
> +        if (prefWidth > biggestPrefWidth)
>            biggestPrefWidth = prefWidth;
>  
> -        if (minWidth > biggestMinWidth) 
> +        if (minWidth > biggestMinWidth)
>            biggestMinWidth = minWidth;
>  
> -        if (maxWidth < smallestMaxWidth) 
> +        if (maxWidth < smallestMaxWidth)

Ideally, the whitespace cleanup here should be split into its own cset.

Not a huge deal though. r=me either way.
Attachment #8876905 - Flags: review?(dholbert) → review+
Looks like this unused variable dates back to this commit from The Year Two Thousand:
http://searchfox.org/mozilla-central/commit/a794898863568ecbbdbf2893ea08ea79e7de76d7
Commit message "Putting back code that was backed out last week"

And at that point [where the variable was created], the variable was unused.

Perhaps it was previously used in the previous "last week" iteration of this cset, before it was backed out -- who knows. :) I didn't bother digging to find out.

Anyway, seems over-ripe for removal.
Comment on attachment 8876905 [details]
Bug 1372386 - nsSprocketLayout.cpp: Remove useless declaration

https://reviewboard.mozilla.org/r/148222/#review152990

Thanks for splitting out the whitespace changes. That makes it easier to hone in on the actual code changes, and that clued me in to one possible problem here...

::: layout/xul/nsSprocketLayout.cpp
(Diff revision 2)
>        currentBox = new (aState) nsBoxSize();
>        if (!aBoxSizes) {
>          aBoxSizes = currentBox;
> -        last = aBoxSizes;
> -      } else {
> -        last->next = currentBox;

Hmm... It looks like this "last->next" assignment might have observable effects.

For example, at the bottom of this loop, we do:

  currentBox = currentBox->next;

In the event that "last" is (or was) pointing to the same thing as currentBox, then this patch would change our behavior. There may be other usages of "next" that would matter, too.

Though maybe you've noticed this as well & proven that this assignment doesn't make a difference? (If so, please include a mention/explanation of that in the changeset.)  Otherwise, I suspect we can't take this patch after all since it seems |last| isn't unused after all.
Attachment #8876905 - Flags: review+ → review-
Comment on attachment 8877055 [details]
Bug 1372386 - Remove trailing whitespace

https://reviewboard.mozilla.org/r/148402/#review152994

r=me on the whitespace patch (thanks for the cleanup!)
Attachment #8877055 - Flags: review?(dholbert) → review+
Comment on attachment 8876905 [details]
Bug 1372386 - nsSprocketLayout.cpp: Remove useless declaration

https://reviewboard.mozilla.org/r/148222/#review153074

::: layout/xul/nsSprocketLayout.cpp
(Diff revision 3)
>      if (!currentBox) {
>        // create one.
>        currentBox = new (aState) nsBoxSize();
>        if (!aBoxSizes) {
>          aBoxSizes = currentBox;
> -        last = aBoxSizes;

Can you explain why this is useless?  It's non-obvious to me.

This is in a loop, and even though our "last" value isn't used on this loop-iteration, maybe it might be used on the next iteration of this loop? (If our next loop iteration takes the other branch and does "last->next = currentBox", then our last iteration's "last" assignment will make a difference.)

(Also, even if this particular assignment *were* useless [which I'm not sure of], it still might be worth doing for consistency, so that "last" always tracks a reliably grokkable thing.  I'm not sure; I'd need to stare at this code for a little while to be able to usefully reason about this.)
Attachment #8876905 - Flags: review?(dholbert) → review-
Comment on attachment 8876905 [details]
Bug 1372386 - nsSprocketLayout.cpp: Remove useless declaration

https://reviewboard.mozilla.org/r/148222/#review152990

> Hmm... It looks like this "last->next" assignment might have observable effects.
> 
> For example, at the bottom of this loop, we do:
> 
>   currentBox = currentBox->next;
> 
> In the event that "last" is (or was) pointing to the same thing as currentBox, then this patch would change our behavior. There may be other usages of "next" that would matter, too.
> 
> Though maybe you've noticed this as well & proven that this assignment doesn't make a difference? (If so, please include a mention/explanation of that in the changeset.)  Otherwise, I suspect we can't take this patch after all since it seems |last| isn't unused after all.

The static analyzer detected that last = aBoxSizes; is useless (line 768) 
Looking at the patch, I was expecting that last was usueless but I might be wrong :)
sorry for the noise, I will just remove this declaration then!
Comment on attachment 8876905 [details]
Bug 1372386 - nsSprocketLayout.cpp: Remove useless declaration

https://reviewboard.mozilla.org/r/148222/#review153074

> Can you explain why this is useless?  It's non-obvious to me.
> 
> This is in a loop, and even though our "last" value isn't used on this loop-iteration, maybe it might be used on the next iteration of this loop? (If our next loop iteration takes the other branch and does "last->next = currentBox", then our last iteration's "last" assignment will make a difference.)
> 
> (Also, even if this particular assignment *were* useless [which I'm not sure of], it still might be worth doing for consistency, so that "last" always tracks a reliably grokkable thing.  I'm not sure; I'd need to stare at this code for a little while to be able to usefully reason about this.)

The checker says the following:
CID 1286390 (#1 of 1): Unused value (UNUSED_VALUE)assigned_pointer: Assigning value from aBoxSizes to last here, but that stored value is overwritten before it can be used.

That sounds legit to me. Anyway, I don't mind keeping it
OK!

Yeah, I'm willing to believe that this particular assignment might be machine-provably useless right now.

But in the absence of a compelling human-understandable explanation for how this code works & why this variable is definitely-useless, I'm inclined to just leave this as it is, in the interests of grokkability and maintainability.  It looks like this variable is aiming to be a "trailing" pointer for the main loop counter (with information from the previous loop iteration).  And if that's the case, it seems worthwhile to keep such a pointer up-to-date, even on loop-iterations where its value is technically not going to used on the next loop iteration.

So: in the absence of such a compelling explanation/case, I'm inclined to just WONTFIX this bug.
Done for you :)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Attachment #8876905 - Attachment is obsolete: true
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6537180ddb8e
Remove trailing whitespace r=dholbert
For the record, I would've preferred for the whitespace patch to be split off into its own bug (or a "no bug" changeset).  The patch is trivial, but it's a bit confusing from a consistency/integrity-of-the-world perspective to have patches landing for a bug that's closed as WONTFIX. :)

Not a huge deal, though, and not worth taking action on at this point.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: