[Rpm-maint] [PATCH 3/5] Optimize expandMacro function
pmatilai at laiskiainen.org
Tue Feb 12 04:38:19 UTC 2013
On 02/05/2013 06:31 AM, Alexey Tourbin wrote:
> From: Alexey Tourbin <at at altlinux.ru>
> This change brings the following optimizations:
> - In the beginning of the routine, the input string needs to be copied
> only in non-terminated case.
This particular (and yes, obvious-looking) optimization was already
reverted once because it breaks things:
So unless something else has changed in a way that avoids that breakage
(which I doubt), NAK. It'd deserve a test-case though, and perhaps a
comment in the code as well.
> - When mb->buf is NULL, expensive xcalloc call is replaced with xmalloc.
> It suffices to ensure that mb->buf is (initially) null-terminated at
> mb->tpos, and that mbAppend* routines follow this rule.
> - The inner per-character loop is replaced with call to strchr(s, '%').
> Also note that the "trailing %" case is no longer handled by this part
> of the code (this is now the case of no valid macro name).
> - New routine mbAppendMem now complements mbAppendStr. Also, mbGrow
> routine was factored to improve possibilities for inlining.
NAK, something here causes severe breakage in rpmbuild (a pile of
segfault failures in the test-suite, and its not the above case of
self-undefining macro as that's not tested currently). For example:
[pmatilai at localhost rpm]$ ./rpmbuild -ts
*** glibc detected *** /home/pmatilai/repos/rpm/.libs/lt-rpmbuild:
free(): invalid next size (fast): 0x00000000006c4620 ***
Aborted (core dumped)
[pmatilai at localhost rpm]$
When submitting patches, please make sure "make check" completes without
errors. The test-suite is somewhat picky of how rpm is ./configure'd so
you'll want to get it working with a known-good starting point (git
master is expected to pass with zero errors at all times, or somebody
has screwed up) before hacking further. It's a bit of a PITA as it takes
so long to complete, but its also been a life-saver on numerous occasions...
Also please split the patch to one patch per independent change. The
patch here isn't exactly big in terms of line-count but there's at least
one or two different problems in it. With strict patch-per-change
approach it's just SO much easier to review patches + spot the exact
changes causing the problem(s). Not to mention when it comes to chasing
down some previously unnoticed regression in an obscure corner case a
year from now.
- Panu -
More information about the Rpm-maint