Closed
Bug 1215238
Opened 9 years ago
Closed 9 years ago
Mention the included file path in pre-processed js sources with #includes
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox45 fixed)
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
Attachments
(1 file)
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•9 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 1•9 years ago
|
||
Assignee: nobody → cmanchester
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes.
Attachment #8678397 -
Flags: review?(gps)
Comment 3•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8678397 -
Flags: review?(gps)
Assignee | ||
Comment 4•9 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•9 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)
Updated•9 years ago
|
Attachment #8678397 -
Flags: review?(mh+mozilla)
Comment 6•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8678397 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 8•9 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 9•9 years ago
|
||
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 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•