Closed Bug 928725 Opened 11 years ago Closed 11 years ago

Use of octal number is deprecated (JavaScript/ECMAScript) and so do not used it in xpcshell-tests files.

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: ishikawa, Assigned: ishikawa)

Details

Attachments

(1 file)

Removing octal literal usage in xpcshell-tests files.
13.60 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
After running |xpcshell-tests| of comm-central thunderbird (DEBUG BUILD), I got the following warnings. TEST-UNEXPECTED-FAIL | /REF-OBJ-DIR/objdir-tb3/mozilla/_tests/xpcshell/toolkit/mozapps/update/test/unit/test_0113_general.js | test failed (with xpcshell return code: 0), see following log: >>>>>>> System JS : WARNING /REF-OBJ-DIR/objdir-tb3/mozilla/_tests/xpcshell/toolkit/mozapps/update/test/unit/head_update.js:1426 - in strict mode code, functions may be declared only at top level or immediately within another function System JS : WARNING /REF-OBJ-DIR/objdir-tb3/mozilla/_tests/xpcshell/toolkit/mozapps/update/test/unit/head_update.js:1429 - octal literals and octal escape sequences are deprecated System JS : WARNING /REF-OBJ-DIR/objdir-tb3/mozilla/_tests/xpcshell/toolkit/mozapps/update/test/unit/head_update.js:1429 - octal literals and octal escape sequences are deprecated System JS : WARNING /REF-OBJ-DIR/objdir-tb3/mozilla/_tests/xpcshell/toolkit/mozapps/update/test/unit/head_update.js:1429 - octal literals and octal escape sequences are deprecated [...] It seems that the recent ECMAScript specification has deprecated octal literal and JS implementation in TB/FF has followed suit. So we must eliminate the octal number usage. Patch is attached. Removing spurious warning lines from octal literal strings is good. After applying the patch and removed the voluminous warning lines from the run, I could notice a real problem / error reported in the log. This is being filed as another bug. TIA
Can someone suggest who the reviewer should be? TIA
Summary: Use of octal number is deprecated (JavaScript/ECMAScript) and so do not used it in xpishll-tests files. → Use of octal number is deprecated (JavaScript/ECMAScript) and so do not used it in xpishell-tests files.
Wouldn't parseInt("n",8) be cleaner than doing the conversion manually everywhere?
(In reply to Dave Garrett from comment #2) > Wouldn't parseInt("n",8) be cleaner than doing the conversion manually > everywhere? Darn. You are right! I would modify the patch. (I am not a regular JavaScript programmer and it shows.) TIA BTW, don't people read the output of test log? I am a little puzzled since this warning must have been there for a few years now.
Of course, the "Right Way" to do this would be to use the actual flags to put together the permissions, but the docs say no one ever bothered to make that easily doable. :/ https://developer.mozilla.org/en-US/docs/PR_Open#Parameters Bug 433295 appears to be the relevant issue there.
(In reply to ISHIKAWA, Chiaki from comment #3) > (In reply to Dave Garrett from comment #2) > > Wouldn't parseInt("n",8) be cleaner than doing the conversion manually > > everywhere? > > Darn. You are right! I would modify the patch. (I am not a regular > JavaScript programmer and it shows.) > > TIA > Well, on second thought, not so fast. The value in question seems to be in an initialize statement, and I wonder if we can use a non-compile-time constant such as parseInt("n",8)? (In reply to Dave Garrett from comment #4) > Of course, the "Right Way" to do this would be to use the actual flags to > put together the permissions, but the docs say no one ever bothered to make > that easily doable. :/ > > https://developer.mozilla.org/en-US/docs/PR_Open#Parameters > > Bug 433295 appears to be the relevant issue there. Yes, it seems relevant, but then I wonder what would be the right solution in the long run. I am afraid that the particular patch posted here may have to be it for the moment. We can think of creating a javascript entity to access such information as the permission for file creation, etc. We need a grand design approach here of making C++ (POSIX binding is offered in C binding mainly and so there are data types and macro constants that we must pass from C/C++ side to JavaScript side in order to pass basic system-dependent information.) What do people think? TIA Actually, I myself found that I/O error code that is so well familiar to posix programmers are no where to be found from the viewpoint of JavaScript and I am afraid that this led to the insufficient checking of I/O operation failure (if you can't figure out exactly what is the nature of the error, why bother checking it?!). The code in the first half of the following patch tried to implement the error code -> meaningful English short message string: https://bug567585.bugzilla.mozilla.org/attachment.cgi?id=705863 The following is a discussion on the matter: https://bugzilla.mozilla.org/show_bug.cgi?id=567585#c41 Such I/O error needs to be caught to handle the case discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=567585#c42 I am trying to fix the bug 567585, now in bug 928250, but to make the initial patch shorter, I have not incorporated this I/O error -> string mapping there yet.
(In reply to Dave Garrett from comment #4) > Of course, the "Right Way" to do this would be to use the actual flags to > put together the permissions, but the docs say no one ever bothered to make > that easily doable. :/ > > https://developer.mozilla.org/en-US/docs/PR_Open#Parameters > > Bug 433295 appears to be the relevant issue there. Should I submit a proposal for inclusion of permission flag values as a separate entry and propose that we incorporate the flag constant somewhere, in a .jsm file such as Fileutils.jsm? Fileutils.jsm may not be the right place, but I think it is better to have these flags anywahere than not having one. Back to the original patch here. I put Joel Maher as a reviewer after looking at https://wiki.mozilla.org/Modules/All#Core TIA
Comment on attachment 819469 [details] [diff] [review] Removing octal literal usage in xpcshell-tests files. I think we can not use non-compile time expression such as ParseInt("0767", 8) in these constant expressions so the mundane replacement is necessary.
Attachment #819469 - Flags: review?(jmaher)
> xpishell-tests xpcshell-tests?
Comment on attachment 819469 [details] [diff] [review] Removing octal literal usage in xpcshell-tests files. Review of attachment 819469 [details] [diff] [review]: ----------------------------------------------------------------- is there no constant we can use?
Attachment #819469 - Flags: review?(jmaher) → review+
(In reply to Masatoshi Kimura [:emk] from comment #8) > > xpishell-tests > > xpcshell-tests? Right. $ make xpcshell-tests Have you considered running xpcshell tests via |mach xpcshell-test|? mach is easier to use and has more features than make and it will eventually be the only way to run xpcshell tests. Please consider using mach today!
(In reply to Joel Maher (:jmaher) from comment #9) > Comment on attachment 819469 [details] [diff] [review] > Removing octal literal usage in xpishell-tests files. > > Review of attachment 819469 [details] [diff] [review]: > ----------------------------------------------------------------- > > is there no constant we can use? Thank you for your review and approval. From what I read in comment 4 from Dave Garrett, I don't think so. Shall I file a bug to make such constants available to JavaScript writers? In that case, please advise me to which file I should add such constants. I will put checkin-needed keyword. Thanks. TIA
Keywords: checkin-needed
Summary: Use of octal number is deprecated (JavaScript/ECMAScript) and so do not used it in xpishell-tests files. → Use of octal number is deprecated (JavaScript/ECMAScript) and so do not used it in xpcshell-tests files.
Attachment #819469 - Attachment description: Removing octal literal usage in xpishell-tests files. → Removing octal literal usage in xpcshell-tests files.
Assignee: nobody → ishikawa
No need for new bugzilla entry to make constants available to JavaScript writers. I found Bug 433295 - Make prio.h constants easier to access from JS TIA
(In reply to ISHIKAWA, Chiaki from comment #12) > No need for new bugzilla entry to make constants available to JavaScript > writers. > I found Bug 433295 - Make prio.h constants easier to access from JS > TIA Or Bug 750178 - [OS.File] Export OS.Constants to the main thread Oh, wait, this one seems to use XPCOM interface, which may be an overkill for just importing CONST values.
(Manually added bug # to the commit message, since was missing; I find using bzexport helpful, since it can add things like that for you :-))
(In reply to Ed Morley [:edmorley UTC+1] from comment #14) > (Manually added bug # to the commit message, since was missing; I find using > bzexport helpful, since it can add things like that for you :-)) Thank you. I will check the commit message next time. (I initially thought this was a feedback to see how the patch would look in the eyes. I better produce the final patch-like submission from the start, I guess.) TIA
https://hg.mozilla.org/mozilla-central/rev/23eb32b0f555
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
BTW: I am planning on switching to these in the update tests so please don't patch them if you are planning on fixing additional cases of these. Thanks!
(In reply to Robert Strong [:rstrong] (do not email) from comment #18) > The 0o prefix for octal literals is IMO a cleaner and better way Of course, but is that even supported here? I can't find any documentation for "0o" octal notation on MDN. This is new notation, right?
It was added in bug 894026 which landed on 07-26 and is still dev-doc-needed. There is only a blog post by waldo atm http://whereswalden.com/2013/08/12/micro-feature-from-es6-now-in-firefox-aurora-and-nightly-binary-and-octal-numbers/
(In reply to Robert Strong [:rstrong] (do not email) from comment #18) > The 0o prefix for octal literals is IMO a cleaner and better way > http://mxr.mozilla.org/mozilla-central/source/modules/libjar/zipwriter/test/ > unit/test_zippermissions.js#56 > > http://mozilla.6506.n7.nabble.com/Octal-literals-have-their-uses-you-Unix- > haters-skip-this-one-td107754.html Thank you for the info. When I was looking for the situation regarding literal octal, I typed ECMAscript octal literal Firefox search engine, and came up with http://mozilla.6506.n7.nabble.com/Octal-literals-have-their-uses-you-Unix-haters-skip-this-one-td107754.html http://interglacial.com/javascript_spec/annexb.html So I thought 0o might be in the standard in the future, but I could not tell. (And I have no idea if 0o was supported now by JS implementation by mozilla's JS.) I advise that we may want to stay away from non-standard features since some JS developers start to use the non-standard features for web works if we allow such constructs in mozilla JS code. (I mean, I would start using such constructs by mimicking the usage in code I read daily.) At least, as others have pointed out, its official status, caution about the "0o" usage (compatibility, standard compliance, etc.) ought to be documented. If "0o" is being proposed to ECMAscript, that is fine. At the same time, I think the needs for passing various symbolic values available on C/C++ side to JS side are still valid, but I think that is for another set of bugzilla entries, or maybe a meta bugzilla entry. TIA
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: