Good for me:
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.15
diff -u -p -r1.15 process.c
--- process.c 27 Oct 2009 23:59:43 -0000 1.15
+++ process.c 18 Jun 2011 21:04:39 -0000
@@ -336,31 +336,18 @@ substitute(struct s_command *cp)
switch (n) {
case 0: /* Global */
do {
- if (lastempty || match[0].rm_so != match[0].rm_eo) {
- /* Locate start of replaced string. */
- re_off = match[0].rm_so;
- /* Copy leading retained string. */
- cspace(&SS, s, re_off, APPEND);
- /* Add in regular expression. */
+ if (match[0].rm_so)
+ cspace(&SS, s, match[0].rm_so, APPEND);
+ if (match[0].rm_so == match[0].rm_eo) {
regsub(&SS, s, cp->u.s->new);
- }
-
- /* Move past this match. */
- if (match[0].rm_so != match[0].rm_eo) {
- s += match[0].rm_eo;
- slen -= match[0].rm_eo;
- lastempty = 0;
+ cspace(&SS, s, match[0].rm_eo++, APPEND);
} else {
- if (match[0].rm_so == 0)
- cspace(&SS, s, match[0].rm_so + 1,
- APPEND);
- else
- cspace(&SS, s + match[0].rm_so, 1,
- APPEND);
- s += match[0].rm_so + 1;
- slen -= match[0].rm_so + 1;
- lastempty = 1;
+ if (match[0].rm_so || match[0].rm_so !=
match[0].rm_eo)
+ regsub(&SS, s, cp->u.s->new);
}
+ /* Move past this match. */
+ s += match[0].rm_eo;
+ slen -= match[0].rm_eo;
} while (slen > 0 && regexec_e(re, s, REG_NOTBOL, 0, slen));
/* Copy trailing retained string. */
if (slen > 0)
2011/6/18 sven falempin <[email protected]>
> Hello,
>
> Well, i failed to apply your second patch :s (patch ./process.c ./yourdiff)
> doing it manually (twice), it pass test but does not correct the initial
> problem.
> # echo 'aba' | sed -E 's/a|$/x/g'
> xbx
>
> Maybe i'm wrong ?
>
> Nevertheless, my attempt is not that much better, it destroy the leading
> \n, i ran the regression test this time.
>
> # echo 'aba' | sed -E 's/a|$/x/g'
> xbxx#
> #
>
> At least, it explain the purpose of lastempty ?
>
> Regards.
>
> Index: process.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/process.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 process.c
> --- process.c 27 Oct 2009 23:59:43 -0000 1.15
> +++ process.c 18 Jun 2011 20:50:28 -0000
> @@ -336,31 +336,15 @@ substitute(struct s_command *cp)
> switch (n) {
> case 0: /* Global */
> do {
> - if (lastempty || match[0].rm_so != match[0].rm_eo)
> {
> - /* Locate start of replaced string. */
> - re_off = match[0].rm_so;
> - /* Copy leading retained string. */
> - cspace(&SS, s, re_off, APPEND);
> - /* Add in regular expression. */
> + if (match[0].rm_so)
> + cspace(&SS, s, match[0].rm_so, APPEND);
> + if (match[0].rm_so == match[0].rm_eo)
> + cspace(&SS, s, match[0].rm_eo++, APPEND);
> + if (match[0].rm_so || match[0].rm_so !=
> match[0].rm_eo)
> regsub(&SS, s, cp->u.s->new);
> - }
> -
> /* Move past this match. */
> - if (match[0].rm_so != match[0].rm_eo) {
> - s += match[0].rm_eo;
> - slen -= match[0].rm_eo;
> - lastempty = 0;
> - } else {
> - if (match[0].rm_so == 0)
> - cspace(&SS, s, match[0].rm_so + 1,
> - APPEND);
> - else
> - cspace(&SS, s + match[0].rm_so, 1,
> - APPEND);
> - s += match[0].rm_so + 1;
> - slen -= match[0].rm_so + 1;
> - lastempty = 1;
> - }
> + s += match[0].rm_eo;
> + slen -= match[0].rm_eo;
> } while (slen > 0 && regexec_e(re, s, REG_NOTBOL, 0,
> slen));
> /* Copy trailing retained string. */
> if (slen > 0)
>
>
>
> 2011/6/18 Ingo Schwarze <[email protected]>
>
>> Hi Otto,
>>
>> from careful code inspection, your patch looks correct.
>>
>> Replacing the ">" by ">=" is wrong, it would breaks this test case:
>>
>> echo abc | ./obj/sed -E 's/(b|())/x/g'
>>
>> However, the code is hard to follow, and i suggest the following
>> refactoring such that people doing a review can more easily check
>> the correctness of the code after application of the patch:
>>
>> 1) X || Y || (!Y && Z) <=> X || Y || Z
>> so leave out the middle + line of your patch.
>>
>> 2) In case of match[0].rm_so == 0,
>> the cspace in the if block you touch has no effect
>> because copying zero bytes would be pointless.
>> Thus, we can pull the cspace before the main if,
>> optionally with an if (match[0].rm_so).
>> That's also much easier to understand:
>> *Always* copy the leading retained string, if any.
>>
>> 3) In the else clause of "Move past this match",
>> the if is completely pointless.
>> For vanishing rm_so, both clauses do the same thing.
>>
>> 4) Pull out the update of s and slen at the end.
>> In the if checking whether the current match is empty,
>> increment rm_eo exactly at the same time as copying the
>> additional character.
>>
>> The resulting code is much shorter and simpler,
>> and hence much easier to audit for correctness.
>>
>> However, i'm a bit scared about touching this code.
>> Breaking this would be really bad.
>> I'm currently running a make build with it.
>>
>> Yours,
>> Ingo
>>
>>
>> > Index: process.c
>> > ===================================================================
>> > RCS file: /cvs/src/usr.bin/sed/process.c,v
>> > retrieving revision 1.15
>> > diff -u -p -r1.15 process.c
>> > --- process.c 27 Oct 2009 23:59:43 -0000 1.15
>> > +++ process.c 15 Jun 2011 06:31:08 -0000
>> > @@ -336,7 +336,9 @@ substitute(struct s_command *cp)
>> > switch (n) {
>> > case 0: /* Global */
>> > do {
>> > - if (lastempty || match[0].rm_so != match[0].rm_eo)
>> {
>> > + if (lastempty || match[0].rm_so != match[0].rm_eo
>> ||
>> > + (match[0].rm_so == match[0].rm_eo &&
>> > + match[0].rm_so > 0)) {
>> > /* Locate start of replaced string. */
>> > re_off = match[0].rm_so;
>> > /* Copy leading retained string. */
>>
>>
>> Index: process.c
>> ===================================================================
>> RCS file: /cvs/src/usr.bin/sed/process.c,v
>> retrieving revision 1.15
>> diff -u -p -r1.15 process.c
>> --- process.c 27 Oct 2009 23:59:43 -0000 1.15
>> +++ process.c 18 Jun 2011 18:23:00 -0000
>> @@ -336,31 +336,25 @@ substitute(struct s_command *cp)
>> switch (n) {
>> case 0: /* Global */
>> do {
>> - if (lastempty || match[0].rm_so != match[0].rm_eo)
>> {
>> - /* Locate start of replaced string. */
>> - re_off = match[0].rm_so;
>> - /* Copy leading retained string. */
>> - cspace(&SS, s, re_off, APPEND);
>> - /* Add in regular expression. */
>> + /* Copy leading retained string. */
>> + if (match[0].rm_so)
>> + cspace(&SS, s, match[0].rm_so, APPEND);
>> +
>> + /* Append replacement. */
>> + if (lastempty || match[0].rm_so ||
>> + match[0].rm_so != match[0].rm_eo)
>> regsub(&SS, s, cp->u.s->new);
>> - }
>>
>> - /* Move past this match. */
>> - if (match[0].rm_so != match[0].rm_eo) {
>> - s += match[0].rm_eo;
>> - slen -= match[0].rm_eo;
>> - lastempty = 0;
>> - } else {
>> - if (match[0].rm_so == 0)
>> - cspace(&SS, s, match[0].rm_so + 1,
>> - APPEND);
>> - else
>> - cspace(&SS, s + match[0].rm_so, 1,
>> - APPEND);
>> - s += match[0].rm_so + 1;
>> - slen -= match[0].rm_so + 1;
>> + if (match[0].rm_so == match[0].rm_eo) {
>> + cspace(&SS, s + match[0].rm_eo++, 1,
>> APPEND);
>> lastempty = 1;
>> - }
>> + } else
>> + lastempty = 0;
>> +
>> + /* Move past this match. */
>> + s += match[0].rm_eo;
>> + slen -= match[0].rm_eo;
>> +
>> } while (slen > 0 && regexec_e(re, s, REG_NOTBOL, 0,
>> slen));
>> /* Copy trailing retained string. */
>> if (slen > 0)
>>
>>
>
>
> --
>
> ---------------------------------------------------------------------------------------------------------------------
> () ascii ribbon campaign - against html e-mail
> /\
>
>
--
---------------------------------------------------------------------------------------------------------------------
() ascii ribbon campaign - against html e-mail
/\
