[PATCH] Simplify alias storage.

[PATCH] Simplify alias storage.

[Harald van Dijk]

Rather than storing the alias name and value separately, we can reduce
simplify code and reduce code size by storing them in name=value form.
This allows us to re-use some code from var.c to handle hashing and
comparisons, so long as we update that to account for aliases' special
handling of a leading = character. This is okay to do for variables as
well, as for variables the leading character is guaranteed to not be =.
---
  src/alias.c | 32 ++++++++++----------------------
  src/var.c   | 27 +++++++++++----------------
  src/var.h   | 15 +++++++++++++++
  3 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/src/alias.c b/src/alias.c
index fcad43b..bb963a6 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -41,6 +41,7 @@
  #include "mystring.h"
  #include "alias.h"
  #include "options.h"	/* XXX for argptr (should remove?) */
+#include "var.h"
   #define ATABSIZE 39
  @@ -55,25 +56,26 @@ void
  setalias(const char *name, const char *val)
  {
  	struct alias *ap, **app;
+	size_t namelen;
   	app = __lookupalias(name);
  	ap = *app;
  	INTOFF;
  	if (ap) {
  		if (!(ap->flag & ALIASINUSE)) {
-			ckfree(ap->val);
+			ckfree(ap->name);
  		}
-		ap->val	= savestr(val);
  		ap->flag &= ~ALIASDEAD;
  	} else {
  		/* not found */
  		ap = ckmalloc(sizeof (struct alias));
-		ap->name = savestr(name);
-		ap->val = savestr(val);
  		ap->flag = 0;
  		ap->next = 0;
  		*app = ap;
  	}
+	namelen = val - name;
+	ap->name = savestr(name);
+	ap->val = ap->name + namelen;
  	INTON;
  }
  @@ -150,8 +152,7 @@ aliascmd(int argc, char **argv)
  			} else
  				printalias(ap);
  		} else {
-			*v++ = '\0';
-			setalias(n, v);
+			setalias(n, v + 1);
  		}
  	}
  @@ -190,36 +191,23 @@ freealias(struct alias *ap) {
   	next = ap->next;
  	ckfree(ap->name);
-	ckfree(ap->val);
  	ckfree(ap);
  	return next;
  }
   void
  printalias(const struct alias *ap) {
-	out1str(single_quote(ap->name));
-	out1fmt("=%s\n", single_quote(ap->val));
+	out1fmt("%s\n", single_quote(ap->name));
  }
   STATIC struct alias **
  __lookupalias(const char *name) {
-	unsigned int hashval;
  	struct alias **app;
-	const char *p;
-	unsigned int ch;
  -	p = name;
-
-	ch = (unsigned char)*p;
-	hashval = ch << 4;
-	while (ch) {
-		hashval += ch;
-		ch = (unsigned char)*++p;
-	}
-	app = &atab[hashval % ATABSIZE];
+	app = &atab[hashval(name) % ATABSIZE];
   	for (; *app; app = &(*app)->next) {
-		if (equal(name, (*app)->name)) {
+		if (varequal(name, (*app)->name)) {
  			break;
  		}
  	}
diff --git a/src/var.c b/src/var.c
index b70d72c..64d93aa 100644
--- a/src/var.c
+++ b/src/var.c
@@ -625,12 +625,7 @@ void unsetvar(const char *s)
  STATIC struct var **
  hashvar(const char *p)
  {
-	unsigned int hashval;
-
-	hashval = ((unsigned char) *p) << 4;
-	while (*p && *p != '=')
-		hashval += (unsigned char) *p++;
-	return &vartab[hashval % VTABSIZE];
+	return &vartab[hashval(p) % VTABSIZE];
  }
   @@ -644,19 +639,19 @@ hashvar(const char *p)
  int
  varcmp(const char *p, const char *q)
  {
-	int c, d;
-
-	while ((c = *p) == (d = *q)) {
-		if (!c || c == '=')
-			goto out;
+	int c = *p, d = *q;
+	while (c == d) {
+		if (!c)
+			break;
  		p++;
  		q++;
+		c = *p;
+		d = *q;
+		if (c == '=')
+			c = '\0';
+		if (d == '=')
+			d = '\0';
  	}
-	if (c == '=')
-		c = 0;
-	if (d == '=')
-		d = 0;
-out:
  	return c - d;
  }
  diff --git a/src/var.h b/src/var.h
index aa7575a..2aece9d 100644
--- a/src/var.h
+++ b/src/var.h
@@ -153,6 +153,21 @@ int unsetcmd(int, char **);
  void unsetvar(const char *);
  int varcmp(const char *, const char *);
  +static inline unsigned int hashval(const char *p)
+{
+	unsigned int hashval;
+
+	hashval = ((unsigned char) *p) << 4;
+	while (*p) {
+		hashval += (unsigned char) *p++;
+		if (*p == '=')
+			break;
+	}
+
+	return hashval;
+}
+
+
  static inline int varequal(const char *a, const char *b) {
  	return !varcmp(a, b);
  }
-- 
2.39.0

[PATCH][resend] Simplify alias storage.

[Harald van Dijk]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On 11/01/2023 18:23, Harald van Dijk wrote:
> Rather than storing the alias name and value separately, we can reduce
> simplify code and reduce code size by storing them in name=value form.
> This allows us to re-use some code from var.c to handle hashing and
> comparisons, so long as we update that to account for aliases' special
> handling of a leading = character. This is okay to do for variables as
> well, as for variables the leading character is guaranteed to not be =.

Apparently this got messed up when I actually sent it. Re-sending as an 
attachment. Apologies for the noise.

Cheers,
Harald van Dijk

[-- Attachment #2: 0001-Simplify-alias-storage.patch --]
[-- Type: text/x-patch, Size: 4016 bytes --]

From eaff1be753f58741a593e6b6a4035533b242a16d Mon Sep 17 00:00:00 2001
From: Harald van Dijk <harald@gigawatt.nl>
Date: Wed, 11 Jan 2023 14:08:02 +0000
Subject: [PATCH] Simplify alias storage.

Rather than storing the alias name and value separately, we can reduce
simplify code and reduce code size by storing them in name=value form.
This allows us to re-use some code from var.c to handle hashing and
comparisons, so long as we update that to account for aliases' special
handling of a leading = character. This is okay to do for variables as
well, as for variables the leading character is guaranteed to not be =.
---
 src/alias.c | 32 ++++++++++----------------------
 src/var.c   | 27 +++++++++++----------------
 src/var.h   | 15 +++++++++++++++
 3 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/src/alias.c b/src/alias.c
index fcad43b..bb963a6 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -41,6 +41,7 @@
 #include "mystring.h"
 #include "alias.h"
 #include "options.h"	/* XXX for argptr (should remove?) */
+#include "var.h"
 
 #define ATABSIZE 39
 
@@ -55,25 +56,26 @@ void
 setalias(const char *name, const char *val)
 {
 	struct alias *ap, **app;
+	size_t namelen;
 
 	app = __lookupalias(name);
 	ap = *app;
 	INTOFF;
 	if (ap) {
 		if (!(ap->flag & ALIASINUSE)) {
-			ckfree(ap->val);
+			ckfree(ap->name);
 		}
-		ap->val	= savestr(val);
 		ap->flag &= ~ALIASDEAD;
 	} else {
 		/* not found */
 		ap = ckmalloc(sizeof (struct alias));
-		ap->name = savestr(name);
-		ap->val = savestr(val);
 		ap->flag = 0;
 		ap->next = 0;
 		*app = ap;
 	}
+	namelen = val - name;
+	ap->name = savestr(name);
+	ap->val = ap->name + namelen;
 	INTON;
 }
 
@@ -150,8 +152,7 @@ aliascmd(int argc, char **argv)
 			} else
 				printalias(ap);
 		} else {
-			*v++ = '\0';
-			setalias(n, v);
+			setalias(n, v + 1);
 		}
 	}
 
@@ -190,36 +191,23 @@ freealias(struct alias *ap) {
 
 	next = ap->next;
 	ckfree(ap->name);
-	ckfree(ap->val);
 	ckfree(ap);
 	return next;
 }
 
 void
 printalias(const struct alias *ap) {
-	out1str(single_quote(ap->name));
-	out1fmt("=%s\n", single_quote(ap->val));
+	out1fmt("%s\n", single_quote(ap->name));
 }
 
 STATIC struct alias **
 __lookupalias(const char *name) {
-	unsigned int hashval;
 	struct alias **app;
-	const char *p;
-	unsigned int ch;
 
-	p = name;
-
-	ch = (unsigned char)*p;
-	hashval = ch << 4;
-	while (ch) {
-		hashval += ch;
-		ch = (unsigned char)*++p;
-	}
-	app = &atab[hashval % ATABSIZE];
+	app = &atab[hashval(name) % ATABSIZE];
 
 	for (; *app; app = &(*app)->next) {
-		if (equal(name, (*app)->name)) {
+		if (varequal(name, (*app)->name)) {
 			break;
 		}
 	}
diff --git a/src/var.c b/src/var.c
index b70d72c..64d93aa 100644
--- a/src/var.c
+++ b/src/var.c
@@ -625,12 +625,7 @@ void unsetvar(const char *s)
 STATIC struct var **
 hashvar(const char *p)
 {
-	unsigned int hashval;
-
-	hashval = ((unsigned char) *p) << 4;
-	while (*p && *p != '=')
-		hashval += (unsigned char) *p++;
-	return &vartab[hashval % VTABSIZE];
+	return &vartab[hashval(p) % VTABSIZE];
 }
 
 
@@ -644,19 +639,19 @@ hashvar(const char *p)
 int
 varcmp(const char *p, const char *q)
 {
-	int c, d;
-
-	while ((c = *p) == (d = *q)) {
-		if (!c || c == '=')
-			goto out;
+	int c = *p, d = *q;
+	while (c == d) {
+		if (!c)
+			break;
 		p++;
 		q++;
+		c = *p;
+		d = *q;
+		if (c == '=')
+			c = '\0';
+		if (d == '=')
+			d = '\0';
 	}
-	if (c == '=')
-		c = 0;
-	if (d == '=')
-		d = 0;
-out:
 	return c - d;
 }
 
diff --git a/src/var.h b/src/var.h
index aa7575a..2aece9d 100644
--- a/src/var.h
+++ b/src/var.h
@@ -153,6 +153,21 @@ int unsetcmd(int, char **);
 void unsetvar(const char *);
 int varcmp(const char *, const char *);
 
+static inline unsigned int hashval(const char *p)
+{
+	unsigned int hashval;
+
+	hashval = ((unsigned char) *p) << 4;
+	while (*p) {
+		hashval += (unsigned char) *p++;
+		if (*p == '=')
+			break;
+	}
+
+	return hashval;
+}
+
+
 static inline int varequal(const char *a, const char *b) {
 	return !varcmp(a, b);
 }
-- 
2.39.0

Re: [PATCH] Simplify alias storage.

[Herbert Xu]

On Wed, Jan 11, 2023 at 06:23:41PM +0000, Harald van Dijk wrote:
>
>  	if (ap) {
>  		if (!(ap->flag & ALIASINUSE)) {
> -			ckfree(ap->val);
> +			ckfree(ap->name);
>  		}

I think this breaks ALIASINUSE.  When an alias is marked as INUSE,
it will be freed by input.c (popstring).  However, your patch doesn't
touch input.c at all.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Simplify alias storage.

[Harald van Dijk]

On 06/04/2024 08:19, Herbert Xu wrote:
> On Wed, Jan 11, 2023 at 06:23:41PM +0000, Harald van Dijk wrote:
>>
>>   	if (ap) {
>>   		if (!(ap->flag & ALIASINUSE)) {
>> -			ckfree(ap->val);
>> +			ckfree(ap->name);
>>   		}
> 
> I think this breaks ALIASINUSE.  When an alias is marked as INUSE,
> it will be freed by input.c (popstring).  However, your patch doesn't
> touch input.c at all.

You're quite right, thanks for spotting that. A test case that covers 
that is

   alias foo='
   alias foo="echo Hello"
   '
   foo
   foo

Here, the storage of the original 'foo' alias needs to be kept while the 
expansion is ongoing, even when the second 'alias' overwrites the 
original definition. This bug is fixed by below additional patch.

However, the patch conflicts with c8db655b because scanning 'name' will 
now find the '=' character and continue on from that to check the value. 
That should be relatively easy to avoid, but I am not sure it is worth it.

Cheers,
Harald van Dijk

diff --git a/src/input.c b/src/input.c
index 38969a7..4604945 100644
--- a/src/input.c
+++ b/src/input.c
@@ -386,7 +386,7 @@ pushstring(char *s, void *ap)
  	sp->ap = (struct alias *)ap;
  	if (ap) {
  		((struct alias *)ap)->flag |= ALIASINUSE;
-		sp->string = s;
+		sp->string = ((struct alias *)ap)->name;
  	}
  	parsefile->nextc = s;
  	parsefile->nleft = len;
@@ -405,7 +405,7 @@ static void popstring(void)
  		    parsefile->nextc[-1] == '\t') {
  			checkkwd |= CHKALIAS;
  		}
-		if (sp->string != sp->ap->val) {
+		if (sp->string != sp->ap->name) {
  			ckfree(sp->string);
  		}
  	}

Re: [PATCH] Simplify alias storage.

[Herbert Xu]

On Thu, Apr 25, 2024 at 09:46:45PM +0100, Harald van Dijk wrote:
>
> You're quite right, thanks for spotting that. A test case that covers that
> is
> 
>   alias foo='
>   alias foo="echo Hello"
>   '
>   foo
>   foo
> 
> Here, the storage of the original 'foo' alias needs to be kept while the
> expansion is ongoing, even when the second 'alias' overwrites the original
> definition. This bug is fixed by below additional patch.

Could you please post a combined patch? Thanks!

> However, the patch conflicts with c8db655b because scanning 'name' will now
> find the '=' character and continue on from that to check the value. That
> should be relatively easy to avoid, but I am not sure it is worth it.

Don't worry about that one.  I can back it out if necessary.
In fact, I'll probably fix it in a different way as I'm adding
multi-byte support.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2] Simplify alias storage.

[Harald van Dijk]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

On 26/04/2024 03:49, Herbert Xu wrote:
> Could you please post a combined patch? Thanks!

Okay, the attached patch combines both. It applies on top of 177072c2.

Cheers,
Harald van Dijk

[-- Attachment #2: 0001-Simplify-alias-storage.patch --]
[-- Type: text/x-patch, Size: 4623 bytes --]

From 109bf32ad5b14ec3c6d05d97a87f56c233bec5df Mon Sep 17 00:00:00 2001
From: Harald van Dijk <harald@gigawatt.nl>
Date: Sun, 28 Apr 2024 01:38:34 +0100
Subject: [PATCH] Simplify alias storage.

Rather than storing the alias name and value separately, we can reduce
simplify code and reduce code size by storing them in name=value form.
This allows us to re-use some code from var.c to handle hashing and
comparisons, so long as we update that to account for aliases' special
handling of a leading = character. This is okay to do for variables as
well, as for variables the leading character is guaranteed to not be =.
---
 src/alias.c | 32 ++++++++++----------------------
 src/input.c |  4 ++--
 src/var.c   | 27 +++++++++++----------------
 src/var.h   | 15 +++++++++++++++
 4 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/src/alias.c b/src/alias.c
index cee07e9..17e0f1b 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -41,6 +41,7 @@
 #include "mystring.h"
 #include "alias.h"
 #include "options.h"	/* XXX for argptr (should remove?) */
+#include "var.h"
 
 #define ATABSIZE 39
 
@@ -55,25 +56,26 @@ void
 setalias(const char *name, const char *val)
 {
 	struct alias *ap, **app;
+	size_t namelen;
 
 	app = __lookupalias(name);
 	ap = *app;
 	INTOFF;
 	if (ap) {
 		if (!(ap->flag & ALIASINUSE)) {
-			ckfree(ap->val);
+			ckfree(ap->name);
 		}
-		ap->val	= savestr(val);
 		ap->flag &= ~ALIASDEAD;
 	} else {
 		/* not found */
 		ap = ckmalloc(sizeof (struct alias));
-		ap->name = savestr(name);
-		ap->val = savestr(val);
 		ap->flag = 0;
 		ap->next = 0;
 		*app = ap;
 	}
+	namelen = val - name;
+	ap->name = savestr(name);
+	ap->val = ap->name + namelen;
 	INTON;
 }
 
@@ -151,8 +153,7 @@ aliascmd(int argc, char **argv)
 			} else
 				printalias(ap);
 		} else {
-			*v++ = '\0';
-			setalias(n, v);
+			setalias(n, v + 1);
 		}
 	}
 
@@ -191,36 +192,23 @@ freealias(struct alias *ap) {
 
 	next = ap->next;
 	ckfree(ap->name);
-	ckfree(ap->val);
 	ckfree(ap);
 	return next;
 }
 
 void
 printalias(const struct alias *ap) {
-	out1str(single_quote(ap->name));
-	out1fmt("=%s\n", single_quote(ap->val));
+	out1fmt("%s\n", single_quote(ap->name));
 }
 
 STATIC struct alias **
 __lookupalias(const char *name) {
-	unsigned int hashval;
 	struct alias **app;
-	const char *p;
-	unsigned int ch;
 
-	p = name;
-
-	ch = (unsigned char)*p;
-	hashval = ch << 4;
-	while (ch) {
-		hashval += ch;
-		ch = (unsigned char)*++p;
-	}
-	app = &atab[hashval % ATABSIZE];
+	app = &atab[hashval(name) % ATABSIZE];
 
 	for (; *app; app = &(*app)->next) {
-		if (equal(name, (*app)->name)) {
+		if (varequal(name, (*app)->name)) {
 			break;
 		}
 	}
diff --git a/src/input.c b/src/input.c
index 38969a7..4604945 100644
--- a/src/input.c
+++ b/src/input.c
@@ -386,7 +386,7 @@ pushstring(char *s, void *ap)
 	sp->ap = (struct alias *)ap;
 	if (ap) {
 		((struct alias *)ap)->flag |= ALIASINUSE;
-		sp->string = s;
+		sp->string = ((struct alias *)ap)->name;
 	}
 	parsefile->nextc = s;
 	parsefile->nleft = len;
@@ -405,7 +405,7 @@ static void popstring(void)
 		    parsefile->nextc[-1] == '\t') {
 			checkkwd |= CHKALIAS;
 		}
-		if (sp->string != sp->ap->val) {
+		if (sp->string != sp->ap->name) {
 			ckfree(sp->string);
 		}
 	}
diff --git a/src/var.c b/src/var.c
index 895eabc..35ea7c6 100644
--- a/src/var.c
+++ b/src/var.c
@@ -622,12 +622,7 @@ void unsetvar(const char *s)
 STATIC struct var **
 hashvar(const char *p)
 {
-	unsigned int hashval;
-
-	hashval = ((unsigned char) *p) << 4;
-	while (*p && *p != '=')
-		hashval += (unsigned char) *p++;
-	return &vartab[hashval % VTABSIZE];
+	return &vartab[hashval(p) % VTABSIZE];
 }
 
 
@@ -641,19 +636,19 @@ hashvar(const char *p)
 int
 varcmp(const char *p, const char *q)
 {
-	int c, d;
-
-	while ((c = *p) == (d = *q)) {
-		if (!c || c == '=')
-			goto out;
+	int c = *p, d = *q;
+	while (c == d) {
+		if (!c)
+			break;
 		p++;
 		q++;
+		c = *p;
+		d = *q;
+		if (c == '=')
+			c = '\0';
+		if (d == '=')
+			d = '\0';
 	}
-	if (c == '=')
-		c = 0;
-	if (d == '=')
-		d = 0;
-out:
 	return c - d;
 }
 
diff --git a/src/var.h b/src/var.h
index 953a694..f6fb320 100644
--- a/src/var.h
+++ b/src/var.h
@@ -153,6 +153,21 @@ int unsetcmd(int, char **);
 void unsetvar(const char *);
 int varcmp(const char *, const char *);
 
+static inline unsigned int hashval(const char *p)
+{
+	unsigned int hashval;
+
+	hashval = ((unsigned char) *p) << 4;
+	while (*p) {
+		hashval += (unsigned char) *p++;
+		if (*p == '=')
+			break;
+	}
+
+	return hashval;
+}
+
+
 static inline int varequal(const char *a, const char *b) {
 	return !varcmp(a, b);
 }
-- 
2.43.0

Re: [PATCH v2] Simplify alias storage.

[Herbert Xu]

On Sun, Apr 28, 2024 at 01:43:37AM +0100, Harald van Dijk wrote:
> 
> Rather than storing the alias name and value separately, we can reduce
> simplify code and reduce code size by storing them in name=value form.
> This allows us to re-use some code from var.c to handle hashing and
> comparisons, so long as we update that to account for aliases' special
> handling of a leading = character. This is okay to do for variables as
> well, as for variables the leading character is guaranteed to not be =.
> ---
>  src/alias.c | 32 ++++++++++----------------------
>  src/input.c |  4 ++--
>  src/var.c   | 27 +++++++++++----------------
>  src/var.h   | 15 +++++++++++++++
>  4 files changed, 38 insertions(+), 40 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt