732762 - pymake: Combine adjacent strings in Expansion.finish()

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Michael Haggerty]

Attachment: Patch that merges adjacent strings together within Expansion elements.

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.

Comment 1

[Ed Morley [:emorley]]

Comment on attachment 602692 [details] [diff] [review]
Patch that merges adjacent strings together within Expansion elements.

Requesting review on behalf of Michael.

Comment 2

[Benjamin Smedberg]

Comment on attachment 602692 [details] [diff] [review]
Patch that merges adjacent strings together within Expansion elements.

This passes tests?

Comment 3

[Michael Haggerty]

(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

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/9fb06df46292