Closed Bug 842438 Opened 10 years ago Closed 10 years ago

Remove @line support

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files, 2 obsolete files)

SpiderMonkey supports "//@line" syntax, which is a bit like #line in C.

We should remove this support, for the following reasons.

- The semantics are unclear, especially if a filename is given.  (E.g. see bug 663934.)

- It's only used in the browser for files that go through Preprocessor.py, which isn't is compelling use... *especially* considering that @line support is disabled in the browser(!)

- JS source maps are the new, better way to do this kind of thing.
Blocks: 747831
Blocks: 779233
These lines no longer do anything.  glandium, in bug 797322 you enabled these
comments for .jsm files -- was that just for consistency?  Presumably, since
atline support has been disabled in SpiderMonkey for a while.  Also, using
//@line comments in Java code doesn't achieve anything, AFAIK...
Attachment #715314 - Flags: review?(mh+mozilla)
Comment on attachment 715314 [details] [diff] [review]
(part 2) - Don't generate @line comments in Preprocessor.py.

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

On a purely code level, this is r+, but I'm not sure we want to walk that path.
Attachment #715314 - Flags: review?(mh+mozilla) → review+
> On a purely code level, this is r+, but I'm not sure we want to walk that
> path.

Can you explain more?
(In reply to Nicholas Nethercote [:njn] from comment #5)
> > On a purely code level, this is r+, but I'm not sure we want to walk that
> > path.
> 
> Can you explain more?

@line is useful, at the very least for tests.
> @line is useful, at the very least for tests.

It's disabled by default in the browser.  Some of the tests enable it in order to test it, but what's the point of testing a feature that isn't used outside of tests?
(In reply to Nicholas Nethercote [:njn] from comment #7)
> > @line is useful, at the very least for tests.
> 
> It's disabled by default in the browser.  Some of the tests enable it in
> order to test it, but what's the point of testing a feature that isn't used
> outside of tests?

That's the point of bug 797325
> That's the point of bug 797325

I don't see much advantage to the //@line comments even if they (a) are on, and (b) actually work.  The cases I've seen are ones like browser/components/nsBrowserGlue.js, which is pre-processed to give something like $OBJ/dist/Nightly.app/Contents/MacOS/components/nsBrowserGlue.js, and the difference between the two is minor.  What's wrong with the error numbers being reported in terms of the latter file?
There are files with large parts enclosed in #ifdef XP_something. When looking at tbpl logs that makes error lines completely off, and looking at your local copy of the preprocessed file doesn't help. I've had the problem enough that i think it's a problem but not so much as to make me want to actively fix it (but then, this is largely because i haven't touched to preprocessed javascript for a while).
One thing that would improve this process would be to change the preprocessor to insert blank lines in place of the #ifdef material that was left out. That would preserve line numbers in that case. It wouldn't work for #includes, but those seem to be more rare.
Attachment #715313 - Flags: review?(benjamin) → review+
(In reply to Bill McCloskey (:billm) from comment #11)
> One thing that would improve this process would be to change the
> preprocessor to insert blank lines in place of the #ifdef material that was
> left out. That would preserve line numbers in that case. It wouldn't work
> for #includes, but those seem to be more rare.

Yes, they are rarer.  Interestingly enough, they cause Preprocessor.py to include @lines for multiple files in a single generated .js file.  SpiderMonkey currently doesn't correctly handle that case -- IIRC it just records the first (or is it the last?) file seen in an @line and uses that file for the whole script.  So the error messages in that case would be totally busted anyway even with @line support enabled.  What a mess.

So my inclination is to add the whitespace as billm suggested.  That way, in files that don't use #include, we'll end up with correct line numbers.  In files that do use #include the line numbers/filenames will be broken, but that's true even if @line is enabled.

And then I can remove @line support, and if someone wants to fix the #include case properly they can use source maps, once they're supported.
This patch replaces line removed by the preprocessor with comment lines, so
that the line number match up.  An example... lines 21--23 of browser.js look
like this:

  #ifndef XP_MACOSX
  var gEditUIVisible = true; 
  #endif

The processed output looks like this:

  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:21
  var gEditUIVisible = true;
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:23

I chose to include the filename/linenum instead of a blank line because it's
more useful.

When #includes are present, the comments are less helpful.  E.g. from
browser.js lines 7437--7444:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  #ifdef XP_WIN
    // Only show resizers on Windows 2000 and XP
    return parseFloat(Services.sysinfo.getProperty("version")) < 6;
  #else 
    return false;
  #endif
  });

becomes lines 14480--14487 of the processed file:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7438
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7439
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7440
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7441
    return false; 
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7443
  });

because there are 7043 lines pulled in prior to this via #include.  So error
numbers reported won't be accurate, but at least it's easy to see how the
processed file was produced from the input files.
Attachment #721560 - Flags: review?(mh+mozilla)
This patch replaces line removed by the preprocessor with comment lines, so
that the line number match up.  An example... lines 21--23 of browser.js look
like this:

  #ifndef XP_MACOSX
  var gEditUIVisible = true; 
  #endif

The processed output looks like this:

  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:21
  var gEditUIVisible = true;
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:23

I chose to include the filename/linenum instead of a blank line because it's
more useful.

When #includes are present, the comments are less helpful.  E.g. from
browser.js lines 7437--7444:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  #ifdef XP_WIN
    // Only show resizers on Windows 2000 and XP
    return parseFloat(Services.sysinfo.getProperty("version")) < 6;
  #else 
    return false;
  #endif
  });

becomes lines 14480--14487 of the processed file:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7438
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7439
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7440
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7441
    return false; 
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7443
  });

because there are 7043 lines pulled in prior to this via #include.  So error
numbers reported won't be accurate, but at least it's easy to see how the
processed file was produced from the input files.
Attachment #721561 - Flags: review?(mh+mozilla)
Attachment #721560 - Attachment is obsolete: true
Attachment #721560 - Flags: review?(mh+mozilla)
Attachment #715314 - Attachment is obsolete: true
Comment on attachment 721561 [details] [diff] [review]
(part 2) - Don't generate @line comments in Preprocessor.py.

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

That's probably going to make some files substantially bigger. Have you measured the difference, especially on mobile?
Attachment #721561 - Flags: review?(mh+mozilla)
> That's probably going to make some files substantially bigger. Have you
> measured the difference, especially on mobile?

No.

BTW, fixing bug 747831 is a high priority for the upcoming GDC (games) conference, and this bug is blocking it.  As a result, my current plan is to land patch 1 soon and worry about patch 2 later :/
Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22835e17f50a

glandium, the ball is in your court w.r.t. whether Preprocessor.py gets changed.  (My inclination is to land the obsoleted patch which just removes the @line comments altogether, and then wait for source maps.)
https://hg.mozilla.org/mozilla-central/rev/22835e17f50a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.