771084 - JavaScript Warning: "TypeError: function FilterPrefs does not always return a value" {file: "chrome://global/content/config.js"

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1341473203.1341476269.12844.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/07/05 00:26:43
{
JavaScript strict warning: chrome://global/content/config.js, line 423: function FilterPrefs does not always return a value
JavaScript strict warning: chrome://global/content/config.js, line 410: function FilterPrefs does not always return a value
}

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: FilterPrefs always returns a value

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Any suggestion for a reviewer?

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 639277 [details] [diff] [review]
FilterPrefs always returns a value

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Any suggestion for a reviewer?

Bug 562640 reviewer.

::: toolkit/components/viewconfig/content/config.js
@@ +408,5 @@
>  
>  function FilterPrefs()
>  {
>    if (document.getElementById("configDeck").getAttribute("selectedIndex") != 1) {
> +    return false ;

Nit: no extra space.

@@ +419,5 @@
>      try {
>        gFilter = RegExp(r[1], r[2]);
>      }
>      catch (e) {
> +      return true; // Do nothing on incomplete or bad RegExp

You should be explicit wrt why this becomes 'true' instead of "false".

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> @@ +419,5 @@
> >      try {
> >        gFilter = RegExp(r[1], r[2]);
> >      }
> >      catch (e) {
> > +      return true; // Do nothing on incomplete or bad RegExp
> 
> You should be explicit wrt why this becomes 'true' instead of "false".

This function is used in two scenarios:
- called as a function, ignoring its result;
- called as a DOM event handler.

Barring any mistake, in a DOM event handler, returning anything other than |false|, including nothing at all, is equivalent. Am I wrong?

Comment 6

[Dão Gottwald [:dao]]

FilterPrefs should never return a value. The return value is never being looked at.

Comment 7

[ISHIKAWA, Chiaki]

Sorry to get in the discussion from the side.

For maintenance purposes, and debugging purposes, it is best
to remove as many warning messages as possible.

I would vote for adding "return" statement with whatever value, it can be even
"undefined" especially if FilterPrefs's return value is not looked at. 

Again, this has nothing to do with the manner a particular function
like FilterPrefs is written.

As of now, debug build of thunderbird has so many warning output and
removing frivolous warning is utmost pressing matter IMHO.
(See Bug 826732 - JavaScript strict warning seen during "make mozmill" run of TB (DEBUG BUILD) )

so something like with appropriate comment as follows

      return appropriatevalue;   // to shut up strict checking of JS interpreter.

in strategic positions would be preferable IMHO.

Again, this is important for long-term maintenance and debugging efforts.
Anyone who looks at DEBUG BUILD output after experiencing a serious bug and
resort to the running of DEBUG BUILD version of TB or FF would agree IMHO.

TIA

Comment 8

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [:dao] from comment #6)
> FilterPrefs should never return a value. The return value is never being
> looked at.

Yoric, do you intend to update the patch here? (I almost created a patch to silence this trivial warning before searching for it in bugzilla, so if you don't I may do it.)

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Go ahead.

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch

Patch as suggested in comment 6.

Comment 11

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2503269f630e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2503269f630e

Comment 13

[:aceman]

(In reply to ISHIKAWA, chiaki from comment #7)
> Sorry to get in the discussion from the side.
> 
> For maintenance purposes, and debugging purposes, it is best
> to remove as many warning messages as possible.
> 
> I would vote for adding "return" statement with whatever value, it can be
> even
> "undefined" especially if FilterPrefs's return value is not looked at. 

Chiaki, nobody is questioning that. Dao just suggested that instead of adding unused return values where they are missing we can remove return values that are already there. The JS warning will go away in either case, it just forces consistency (either all returns have a value in a function, or none have one).

Comment 14

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #13)
> Chiaki, nobody is questioning that. Dao just suggested that instead of
> adding unused return values where they are missing we can remove return
> values that are already there. The JS warning will go away in either case,
> it just forces consistency (either all returns have a value in a function,
> or none have one).
Thank you for the clarification. I looked at the patch and figured that out.
(The new code certainly makes it easier to figure out that we don't need an
explicit return value, and that makes reading the code easier even.)
TIA