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

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 1

[u612829]

Attachment: my-change.patch

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Eric Rahm [: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 4

[Eric Rahm [:erahm]]

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

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Eric Rahm [:erahm]]

(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.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Eric Rahm [:erahm]]

(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.

Comment 9

[Eric Rahm [:erahm]]

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 =`.

Comment 10

[u612829]

Attachment: Remove the first declaration of "start" and set it in each condition.

Comment 11

[u612829]

Attachment: Remove the first declaration of "start" and set it in each condition.

Comment 12

[Eric Rahm [: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`.

Comment 13

[u612829]

Attachment: Remove the first declaration of "start" and set it in each bloc.

Comment 14

[Eric Rahm [: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!

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/01d017ee38c0

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Bravo victor! Your patch should be in the nightly of tomorrow!