Last Comment Bug 732762 - pymake: Combine adjacent strings in Expansion.finish()
: pymake: Combine adjacent strings in Expansion.finish()
Status: RESOLVED FIXED
[pymake]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Michael Haggerty
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-03 22:45 PST by Michael Haggerty
Modified: 2012-03-14 14:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch that merges adjacent strings together within Expansion elements. (2.23 KB, patch)
2012-03-03 22:45 PST, Michael Haggerty
benjamin: review+
Details | Diff | Splinter Review

Description Michael Haggerty 2012-03-03 22:45:08 PST
Created attachment 602692 [details] [diff] [review]
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] 2012-03-04 04:20:41 PST
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 [:bsmedberg] 2012-03-08 10:19:39 PST
Comment on attachment 602692 [details] [diff] [review]
Patch that merges adjacent strings together within Expansion elements.

This passes tests?
Comment 3 Michael Haggerty 2012-03-08 20:56:18 PST
(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 Ted Mielczarek [:ted.mielczarek] 2012-03-09 06:55:40 PST
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 Ted Mielczarek [:ted.mielczarek] 2012-03-09 06:57:20 PST
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.

Note You need to log in before you can comment on or make changes to this bug.