1346859 - Remove dead code found by clang-tidy in unix/nsOSHelperAppService.cpp

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

Clang-tidy found some incorrect code 
http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html 
i guess this is useless

Comment 1

[Sylvestre Ledru [:Sylvestre]]

I guess this is dead code as it was introduced in r1 (2007)

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1346859 - Remove dead code found by clang-tidy in unix/nsOSHelperAppService.cpp

Review commit: https://reviewboard.mozilla.org/r/119750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119750/

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8846743 [details]
Bug 1346859 - Remove dead code found by clang-tidy in unix/nsOSHelperAppService.cpp

https://reviewboard.mozilla.org/r/119750/#review121620

::: uriloader/exthandler/unix/nsOSHelperAppService.cpp
(Diff revision 1)
>        do {
>          --aDescriptionEnd;
>        } while (aDescriptionEnd != aDescriptionStart &&
>                 nsCRT::IsAsciiSpace(*aDescriptionEnd));
> -
> -      if (aDescriptionStart != aDescriptionStart && *aDescriptionEnd == '"') {

In case this isn't obvious, this a != a here

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8846743 [details]
Bug 1346859 - Remove dead code found by clang-tidy in unix/nsOSHelperAppService.cpp

https://reviewboard.mozilla.org/r/119750/#review121624

I'd be interested to hear your thoughts on the below.  I have a slight preference for changing to what was probably meant so some poor soul doesn't waste a bunch of time debugging this...but the dead since hg@1 issue is also a strong argument that nobody cares...?

::: uriloader/exthandler/unix/nsOSHelperAppService.cpp
(Diff revision 1)
>        do {
>          --aDescriptionEnd;
>        } while (aDescriptionEnd != aDescriptionStart &&
>                 nsCRT::IsAsciiSpace(*aDescriptionEnd));
> -
> -      if (aDescriptionStart != aDescriptionStart && *aDescriptionEnd == '"') {

This function.  Such length.  Very wow.

I think this is supposed to be `aDescriptionEnd != aDescriptionStart`, so I kinda want to change it to that...but if this case has been dead since hg@1...

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8846743 [details]
Bug 1346859 - Remove dead code found by clang-tidy in unix/nsOSHelperAppService.cpp

https://reviewboard.mozilla.org/r/119750/#review121624

If you want, I can try this change and push to try. :)
I didn't bother as it has been this way for 10 years now and I think we will create bugs instead of fixing any

Comment 6

[Sylvestre Ledru [:Sylvestre]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=859ec2440236

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Nathan, I tried this: https://hg.mozilla.org/try/rev/859ec24402364438402dbc752255597490f1e51a
And this seems green to me. As it is declared as static, I don't expect it is used by something else.
OK for you?

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Nathan, I tried this:
> https://hg.mozilla.org/try/rev/859ec24402364438402dbc752255597490f1e51a
> And this seems green to me. As it is declared as static, I don't expect it
> is used by something else.
> OK for you?

That's, uh, possibly a little more extensive than what I had in mind.  I have no idea about the ramifications of removing all that code, and I suspect that automated tests aren't going to provide very good coverage here because they don't have the kind of environment that some of that code was written to catch (e.g. opening realplayer files?  opening StarOffice files?  probably not happening in our tests).

Let's go ahead with the original r+'d mozreview patch; we can remove the code in a separate bug if we really think it necessary.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e99d208aa1a
Remove dead code found by clang-tidy in unix/nsOSHelperAppService.cpp r=froydnj

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e99d208aa1a