Mention the included file path in pre-processed js sources with #includes

RESOLVED FIXED in Firefox 45

Status

RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

unspecified
mozilla45

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
For instance, https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/_tests/reftest/b2g_start_script.js doesn't mention reftest-preferences.js (which is where all those prefs came from).
(Assignee)

Updated

3 years ago
Summary: Mention the included file path in pre-processed sources with #includes → Mention the included file path in pre-processed js sources with #includes
(Assignee)

Comment 2

3 years ago
Created attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes.
Attachment #8678397 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/23199/#review20977

::: python/mozbuild/mozbuild/preprocessor.py:769
(Diff revision 1)
> +            self.writtenLines = -1

It seems to me this could, in some cases, add a //@line comment at the beginning of the original file.

Also, it looks to me like the whole thing with "writtenLines" is broken, and that you can find a corner case where a //@line comment wouldn't be printed out when "returning" from an include if the line numbers match somehow.

I think we should instead track the tuple (file, line), and output //@line when that doesn't match what was for the last printed out line.
Attachment #8678397 - Flags: review?(gps)
(Assignee)

Comment 4

3 years ago
https://reviewboard.mozilla.org/r/23199/#review20977

> It seems to me this could, in some cases, add a //@line comment at the beginning of the original file.
> 
> Also, it looks to me like the whole thing with "writtenLines" is broken, and that you can find a corner case where a //@line comment wouldn't be printed out when "returning" from an include if the line numbers match somehow.
> 
> I think we should instead track the tuple (file, line), and output //@line when that doesn't match what was for the last printed out line.

A //@ line would be added at the beginning of a file if a file name were passed as a string to do_include, but this doesn't happen in the build system.

I couldn't replicate the bug proposed because I guess the "#include" line itself is what causes the mismatch when "returning" from the included file. Tracking a tuple still seems a lot clearer, I'll give it a go.
(Assignee)

Comment 5

3 years ago
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium
Attachment #8678397 - Attachment description: MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. → MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium
Attachment #8678397 - Flags: review?(mh+mozilla)
Attachment #8678397 - Flags: review?(mh+mozilla)
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

https://reviewboard.mozilla.org/r/23199/#review21821

::: python/mozbuild/mozbuild/preprocessor.py:410
(Diff revision 2)
> +    def updatedLineInfo(self, line, filename):

updatedLineInfo is a weird name for this function, but I don't have any better suggestion to give. OTOH, the function is only used once, so how about inlining it?

    if self.checkLineNumbers:
        expected_file, expected_line = self.line_info
        if expected_line + 1 != next_line or expected_file and expected_file != next_file:
             ...

::: python/mozbuild/mozbuild/preprocessor.py:767
(Diff revision 2)
> +        self.noteLineInfo()

This is too early for this one. It seems to me this should happen where the writtenLines used to be set.

::: python/mozbuild/mozbuild/preprocessor.py:786
(Diff revision 2)
> +        self.noteLineInfo()

This doesn't seem necessary.

::: python/mozbuild/mozbuild/test/test_preprocessor.py:590
(Diff revision 2)
> +                             ('//@line 1 "CWDfoo.js"\n'
> +                              'bazfoobar\n'
> +                              '//@line 2 "CWDbar.js"\n'
> +                              '@foo@\n'
> +                              '//@line 3 "CWDfoo.js"\n'
> +                              'bazbarfoo\n'
> +                              '//@line 2 "CWDbar.js"\n'
> +                              'foobarbaz\n'
> +                              '//@line 3 "CWDtest.js"\n'
> +                              'barfoobaz\n').replace('CWD',

replace CWD with CWD/, that will make things easier to get when just glancing over the code.
(Assignee)

Comment 7

3 years ago
https://reviewboard.mozilla.org/r/23199/#review21821

> This is too early for this one. It seems to me this should happen where the writtenLines used to be set.

Putting this where writtenLines used to be set means "entering" a file isn't enough to cause a "//@ line" comment, because we get this sequence:

1. self.context['FILE'] is updated to the new line, self.context['LINE'] is set to 0.
2. self.noteLineInfo() <-- sets self.last_line to the _new_ file and line to 0.
3. self.handleLine(l) <-- gets the new file and line 1, which it expects.

I added to the test to show this a little more clearly.
(Assignee)

Updated

3 years ago
Attachment #8678397 - Flags: review?(mh+mozilla)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23199/diff/2-3/
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

https://reviewboard.mozilla.org/r/23199/#review21989

::: python/mozbuild/mozbuild/test/test_preprocessor.py:606
(Diff revision 3)
> +                              'fin\n').replace('CWD', os.getcwd()))

I think you still need to replace CWD/ with os.getcwd() + os.pathsep, because Windows.
Attachment #8678397 - Flags: review?(mh+mozilla) → review+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d419fd3f62ef
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.