Closed
Bug 732762
Opened 13 years ago
Closed 13 years ago
pymake: Combine adjacent strings in Expansion.finish()
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mhagger, Assigned: mhagger)
Details
(Whiteboard: [pymake])
Attachments
(1 file)
2.23 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Currently, if an Expansion consists only of simple strings, then the strings are concatenated and the whole think is replaced with a StringExpansion. But if some of the elements are strings but others are not, adjacent strings are left separate:
$ cat >Makefile
all:
@echo '#### $(CFLAGS) ####'
$ ./mkparse.py Makefile
Parsing Makefile
Rule Exp<Makefile:1:0>('all'): Exp<Makefile:1:0>('')
Command <Expansion with elements: ["@echo '", '#', '#', '#', '#', ' ', VariableRef<Makefile:2:16>(Exp<None>('CFLAGS')), ' #', '#', '#', '#', "'"]>
The attached patch changes Expansion.finish() to merge adjacent strings even if not all of the elements are strings. The output after the patch is
$ ./mkparse.py Makefile
Parsing Makefile
Rule Exp<Makefile:1:0>('all'): Exp<Makefile:1:0>('')
Command <Expansion with elements: ["@echo '#### ", VariableRef<Makefile:2:16>(Exp<None>('CFLAGS')), " ####'"]>
I suggest the following commit message:
Compress adjacent strings in Expansion.finish().
When an Expansion is built up, it is possible for many adjacent
elements to be literal strings. In finish(), compress these into
single strings, and additionally remove any empty literal strings
(along with the old behavior of changing an Expansion that consists of
only literal strings into a StringExpansion object).
As part of the new finish() implementation, it is trivial to check
whether any of the elements were functions. So the "hasfunc" member
variable is no longer needed; get rid of it.
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [pymake]
Comment 1•13 years ago
|
||
Comment on attachment 602692 [details] [diff] [review]
Patch that merges adjacent strings together within Expansion elements.
Requesting review on behalf of Michael.
Attachment #602692 -
Flags: review?(benjamin)
Comment 2•13 years ago
|
||
Comment on attachment 602692 [details] [diff] [review]
Patch that merges adjacent strings together within Expansion elements.
This passes tests?
Attachment #602692 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> Comment on attachment 602692 [details] [diff] [review]
> Patch that merges adjacent strings together within Expansion elements.
>
> This passes tests?
It's not obvious how to run the test suite correctly. (It would be great if this were documented in the README file.) My working hypothesis is
python tests/runtests.py --gmake=/usr/bin/make
, which completes successfully.
Comment 4•13 years ago
|
||
That's correct. Sorry that's not well-documented! The test suite runs each testcase in both GNU make and Pymake (except for specific tests that are annotated to skip one or the other). It defaults to using "gmake" for make, which doesn't quite work on all platforms, we could probably fix that.
Comment 5•13 years ago
|
||
Also FYI, I just gave your Bugzilla account the ability to edit bugs, so you should be able to assign bugs to yourself and fiddle other fields.
Assignee: nobody → mhagger
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [pymake] → [pymake][c-n to pymake]
Comment 6•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [pymake][c-n to pymake] → [pymake]
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
•