Closed Bug 732762 Opened 13 years ago Closed 13 years ago

pymake: Combine adjacent strings in Expansion.finish()

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mhagger, Assigned: mhagger)

Details

(Whiteboard: [pymake])

Attachments

(1 file)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [pymake]
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 on attachment 602692 [details] [diff] [review] Patch that merges adjacent strings together within Expansion elements. This passes tests?
Attachment #602692 - Flags: review?(benjamin) → review+
(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.
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.
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
Keywords: checkin-needed
Whiteboard: [pymake] → [pymake][c-n to pymake]
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [pymake][c-n to pymake] → [pymake]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: