txExprLexer::parse: Value stored to 'start' is never read

RESOLVED FIXED in Firefox 60

Status

()

defect
--
trivial
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: sylvestre, Assigned: miraty)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
Found by scan-build / clang analyzer http://sylvestre.ledru.info/reports/fx-scan-build/report-txExprLexer.cpp-parse-2-1.html#EndPath

Reporting for an intern to test the workflow.
(Assignee)

Comment 1

a year ago
Posted patch my-change.patch (obsolete) — Splinter Review
Attachment #8954383 - Flags: review?(continuation)
Comment on attachment 8954383 [details] [diff] [review]
my-change.patch

Review of attachment 8954383 [details] [diff] [review]:
-----------------------------------------------------------------

erahm is a better reviewer for this code than me. Off hand, I'd think we still need to call BeginReading. I'm not sure if it is better to drop the assignment or to do something with the result.
Attachment #8954383 - Flags: review?(continuation) → review?(erahm)
(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> Found by scan-build / clang analyzer
> http://sylvestre.ledru.info/reports/fx-scan-build/report-txExprLexer.cpp-
> parse-2-1.html#EndPath
> 
> Reporting for an intern to test the workflow.

This warning is clearly wrong; it's wrong to the point I'd suggest not using whatever analyzer you're using.
Comment on attachment 8954383 [details] [diff] [review]
my-change.patch

Review of attachment 8954383 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! The static analysis is wrong so I'm going to r- it indicating we don't want to land this.

It looks like you have the right format for a patch (bug # - title - r? reviewer), but in the future can you provide a more descriptive patch title and perhaps a descriptive comment? Something like the following would be helpful:

> Bug 1441523 - Remove unused assignment to start in txExprLexer::parse r?mccr8
> 
> Static analysis has found the variable `start` is assigned to, but never used. This removes the assignment.

This makes it easier for the reviewer to know what's going on without looking at the full bug and also makes the history clearer so that I can use |hg log| to look at changes to a file and figure out what has happened.

::: dom/xslt/xpath/txExprLexer.cpp
@@ -94,5 @@
>  nsresult
>  txExprLexer::parse(const nsAString& aPattern)
>  {
>    iterator start, end;
> -  start = aPattern.BeginReading(mPosition);

`start` is used several times in this function. In the future I'd suggest searching through the function [1] to make sure the static analysis is correct. Searchfox [1] makes this pretty easy, just left-click the variable and select 'Sticky highlight'

[1] https://searchfox.org/mozilla-central/rev/056a4057575029f58eb69ae89bc7b6c3c11e2e28/dom/xslt/xpath/txExprLexer.cpp#95,98,128,151,170,173,182,196-197,201,204,213,217
Attachment #8954383 - Flags: review?(erahm) → review-
The variable |start| is used, but every use is preceded by another definition, so the assignment from BeginReading() isn't needed, so I think the analysis is right. Honestly, I think it would be better to sink the declaration to all of the places where it is used, as it is not obvious.
(In reply to Andrew McCreight [:mccr8] from comment #5)
> The variable |start| is used, but every use is preceded by another
> definition, so the assignment from BeginReading() isn't needed, so I think
> the analysis is right. Honestly, I think it would be better to sink the
> declaration to all of the places where it is used, as it is not obvious.

Ah I see, that instance is overwritten. As Andrew noted, this is confusing enough that we should keep the assignment. Future changes to the code could end up using an uninitialized variable which is way worse that an extra assignment of a pointer.
Well, if |start| is redeclared every place it is assigned to, and the top level declaration is eliminated, then this wouldn't be a problem. I think having multiple declarations would be ok.
(In reply to Andrew McCreight [:mccr8] from comment #7)
> Well, if |start| is redeclared every place it is assigned to, and the top
> level declaration is eliminated, then this wouldn't be a problem. I think
> having multiple declarations would be ok.

That could work too, but honestly I don't think it's worth the churn, in the same way I don't think pursing these clever-and-technically-correct-but-not-terribly-useful warnings is going to be worth the effort.
miraty, sorry this became a bit more difficult. We're having a bit of a philosophical debate, but I think you just want to land a patch :)

The correct change here to get rid of the warning is to remove the declaration of `start` from line 97, remove the assignment to `start` on line 98 but keep the `aPattern.BeginReading(mPosition);` portion, that assigns a value to `mPosition`. Then for each instance where we assign to `start` in the rest of the function we want you to declare a scope level variable, ie `iterator start =` instead of `start =`.
(Reporter)

Updated

a year ago
Assignee: nobody → miraty
(Assignee)

Comment 10

a year ago
noise
Attachment #8954383 - Attachment is obsolete: true
Attachment #8954733 - Flags: review?(erahm)
(Assignee)

Updated

a year ago
Attachment #8954733 - Attachment is obsolete: true
Attachment #8954733 - Flags: review?(erahm)
(Assignee)

Comment 11

a year ago
Attachment #8954737 - Flags: review?(erahm)
Comment on attachment 8954737 [details] [diff] [review]
Remove the first declaration of "start" and set it in each condition.

Review of attachment 8954737 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one more small tweak.

::: dom/xslt/xpath/txExprLexer.cpp
@@ +181,5 @@
>        }
>        newToken = new Token(start, mPosition, Token::NUMBER);
>      }
>      else {
> +      iterator start;

Lets move the declaration to each assignment below so that we don't have an uninitialized instance of `start`.
Attachment #8954737 - Flags: review?(erahm) → feedback+
(Assignee)

Comment 13

a year ago
Attachment #8954737 - Attachment is obsolete: true
Attachment #8955134 - Flags: review?(erahm)
Comment on attachment 8955134 [details] [diff] [review]
Remove the first declaration of "start" and set it in each bloc.

Review of attachment 8955134 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the patch!
Attachment #8955134 - Flags: review?(erahm) → review+
(Reporter)

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/01d017ee38c0491e353ead6d274e16d658a719f4
Bug 1441523 - Remove the first declaration of "start" and set it in each condition r=erahm

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01d017ee38c0
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Reporter)

Comment 17

a year ago
Bravo victor! Your patch should be in the nightly of tomorrow!
You need to log in before you can comment on or make changes to this bug.