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

RESOLVED FIXED in Firefox 60

Status

()

defect
--
trivial
RESOLVED FIXED
Last year
Last year

People

(Reporter: sylvestre, Assigned: miraty)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Last year
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

Last year
Assignee: nobody → miraty
Assignee

Comment 10

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

Updated

Last year
Attachment #8954733 - Attachment is obsolete: true
Attachment #8954733 - 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

Last year
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+
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/01d017ee38c0
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.