[v2] don't record empty IFS scan regions if not debugging

Commit Message

Martijn Dekker March 23, 2018, 4:05 p.m. UTC

Op 23-03-18 om 15:34 schreef Martijn Dekker:
> Op 23-03-18 om 11:58 schreef Herbert Xu:
>> Thanks, the problem here is that we need to set c to 0 not just
>> when quoted is true but also if sep is 0 since both imply that
>> field splitting is disabled.  Here is an second revision which
>> should fix this by checking (quoted || !sep) instead of just
>> quoted when determining whether we're doing field expansion in $*.
> 
> FWIW, this passes all my tests.

But dash is still recording empty IFS scan regions with nothing for IFS
to split. That still seems strange to me.

It's true that this behaviour has helped expose and fix some bugs in
this instance. However, by the same token, other (future) bugs might be
exposed by avoiding illogical behaviour.

So how about having it both ways? If DEBUG is defined, record empty
split regions. If not, don't waste cycles doing so.

- M.

Comments

Herbert Xu March 26, 2018, 10:36 a.m. UTC | #1

On Fri, Mar 23, 2018 at 05:05:53PM +0100, Martijn Dekker wrote:
>
> So how about having it both ways? If DEBUG is defined, record empty
> split regions. If not, don't waste cycles doing so.

You are trying to save cycles by adding a branch that's going to be
true most of the time, I don't see how that could work performance
wise.

Thanks,

Patch

diff --git a/src/expand.c b/src/expand.c
index c14350c..d6c7946 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -774,7 +774,12 @@  record:
 			if (!quoted)
 				goto end;
 		}
-		recordregion(startloc, expdest - (char *)stackblock(), quoted);
+#ifndef DEBUG
+		if (varlen > 0)
+#endif
+			recordregion(startloc,
+				     expdest - (char *)stackblock(),
+				     quoted);
 		goto end;
 	}