Merged
Conversation
99bbead to
4f3501f
Compare
Eliminate a kwarg 'explain' from the call to Builder() for Mkdir. Arg is not used (maybe it was in the past, though have not found the evidence for this), which meant we had an override dict in this case. Suprisingly, this ended up in the override logic a *lot* in a build of a large sample project - over 2000 extra calls to scons_subst_once that weren't needed. Builder and Executor both have places where they call Override(). While the Override factory indeed returns quickly if the override dict is empty, avoid the function call entirely in this case by checking (there was an old TODO comment to this effect in Builder.__init__.py). The Executor init method declared multiple paramters with mutable defaults, which means they "persist". While there's no evidence this is calling a real problem, eliminate part of this footgun by defaulting overridelist and builder_kw to None and checking for that (targets and sources still default to a mutable empty list, a TBD perhaps). Signed-off-by: Mats Wichmann <mats@linux.com>
4f3501f to
2d907e6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eliminate a kwarg
explainfrom the call toBuilder()forMkdir. Arg is not used (maybe it was in the past, though have not found the evidence for this), which meant we had an override dict in this case. Suprisingly, this ended up in the override logic a lot in a build of a large sample project - over 2000 extra calls toscons_subst_oncethat weren't needed.BuilderandExecutorboth have places where they callOverride(). While theOverridefactory indeed returns quickly if the override dict is empty, avoid the function call entirely in this case by checking (there was an old TODO comment to this effect inBuilder.__init__.py).The
Executorinit method declared multiple paramters with mutable defaults, which means they "persist". While there's no evidence this is calling a real problem, eliminate part of this footgun by defaultingoverridelistandbuilder_kwtoNoneand checking for that (targetsandsourcesstill default to a mutable empty list, a TBD perhaps).There are no user-visible effects to these changes; existing tests have been run and indicate no impacts. Thus, there are no doc or test changes included.
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).