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

RESOLVED FIXED in mozilla21

Status

()

Toolkit
Preferences
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: florian)

Tracking

({regression})

Trunk
mozilla21
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 affected)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
}
Created attachment 639277 [details] [diff] [review]
FilterPrefs always returns a value
Assignee: nobody → dteller
Any suggestion for a reviewer?
(Reporter)

Comment 3

6 years ago
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".
Attachment #639277 - Flags: feedback-
(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?
Flags: needinfo?(sgautherie.bz)

Updated

5 years ago
Duplicate of this bug: 834606
FilterPrefs should never return a value. The return value is never being looked at.
Flags: needinfo?(sgautherie.bz)

Comment 7

5 years ago
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
(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.)
Created attachment 712899 [details] [diff] [review]
Patch

Patch as suggested in comment 6.
Assignee: dteller → florian
Attachment #639277 - Attachment is obsolete: true
Attachment #712899 - Flags: review?(dao)

Updated

5 years ago
Attachment #712899 - Flags: review?(dao) → review+

Updated

5 years ago
Blocks: 826732
https://hg.mozilla.org/mozilla-central/rev/2503269f630e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Comment 13

5 years ago
(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

5 years ago
(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
You need to log in before you can comment on or make changes to this bug.