[PATCH] fileio.c: introduce make_temp_name_buf

[PATCH] fileio.c: introduce make_temp_name_buf

[Alexander Kuleshov]

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

Hello,

The make_temp_name function in a case when BASE64_P is true or when
HAVE_LONG_FILE_NAMES is
not defined builds buffer where first three characters is a PID in
base64 encoding. This leads to some of
code duplication.

Attached patch introduces make_temp_name_buf() function which will do
this functionality and allows to avoid
code duplication.

Thank you

[-- Attachment #2: 0001-fileio.c-introduce-make_temp_name_buf.patch --]
[-- Type: text/x-patch, Size: 2300 bytes --]

From c58d13c750901e9f47c99dd2466e95dabb3730a1 Mon Sep 17 00:00:00 2001
From: Alexander Kuleshov <kuleshovmail@gmail.com>
Date: Wed, 20 Jul 2016 18:33:55 +0600
Subject: [PATCH] fileio.c: introduce make_temp_name_buf

* src/fileio.c: New function - make_temp_name_buf. Use it in the
make_temp_name() function.
---
 src/fileio.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index b1f9d3c..a23a5fc 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -620,6 +620,16 @@ static const char make_temp_name_tbl[64] =
   '4','5','6','7','8','9','-','_'
 };
 
+/* Add 3 characters in base64 encoding to the given buffer */
+
+static char *make_temp_name_buf (char *buf, unsigned num) {
+  buf[0] = make_temp_name_tbl[num & 63], num >>= 6;
+  buf[1] = make_temp_name_tbl[num & 63], num >>= 6;
+  buf[2] = make_temp_name_tbl[num & 63], num >>= 6;
+
+  return buf;
+}
+
 static unsigned make_temp_name_count, make_temp_name_count_initialized_p;
 
 /* Value is a temporary file name starting with PREFIX, a string.
@@ -659,9 +669,7 @@ make_temp_name (Lisp_Object prefix, bool base64_p)
 
   if (base64_p)
     {
-      pidbuf[0] = make_temp_name_tbl[pid & 63], pid >>= 6;
-      pidbuf[1] = make_temp_name_tbl[pid & 63], pid >>= 6;
-      pidbuf[2] = make_temp_name_tbl[pid & 63], pid >>= 6;
+      make_temp_name_buf (pidbuf, pid);
       pidlen = 3;
     }
   else
@@ -669,9 +677,7 @@ make_temp_name (Lisp_Object prefix, bool base64_p)
 #ifdef HAVE_LONG_FILE_NAMES
       pidlen = sprintf (pidbuf, "%"pMd, pid);
 #else
-      pidbuf[0] = make_temp_name_tbl[pid & 63], pid >>= 6;
-      pidbuf[1] = make_temp_name_tbl[pid & 63], pid >>= 6;
-      pidbuf[2] = make_temp_name_tbl[pid & 63], pid >>= 6;
+      make_temp_name_buf (pidbuf, pid);
       pidlen = 3;
 #endif
     }
@@ -705,9 +711,7 @@ make_temp_name (Lisp_Object prefix, bool base64_p)
     {
       unsigned num = make_temp_name_count;
 
-      p[0] = make_temp_name_tbl[num & 63], num >>= 6;
-      p[1] = make_temp_name_tbl[num & 63], num >>= 6;
-      p[2] = make_temp_name_tbl[num & 63], num >>= 6;
+      make_temp_name_buf (p, num);
 
       /* Poor man's congruential RN generator.  Replace with
          ++make_temp_name_count for debugging.  */
-- 
2.8.0.rc3.922.g2bcc146

Re: [PATCH] fileio.c: introduce make_temp_name_buf

[Alan Third]

On Wed, Jul 20, 2016 at 06:44:00PM +0600, Alexander Kuleshov wrote:
> Hello,
> 
> The make_temp_name function in a case when BASE64_P is true or when
> HAVE_LONG_FILE_NAMES is not defined builds buffer where first three
> characters is a PID in base64 encoding. This leads to some of code
> duplication.

I was looking over it and thought the same thing.

> Attached patch introduces make_temp_name_buf() function which will
> do this functionality and allows to avoid code duplication.

This looks OK to me.

I think it would make more sense to only check for base64_p when
HAVE_LONG_FILE_NAMES is defined, though. If it’s not defined, it’s
currently checking it then executing identical code whatever the
outcome.

Once that’s done we’re only duplicating the code once and I don’t know
if it’s worth introducing a new function for that.
-- 
Alan Third

Re: [PATCH] fileio.c: introduce make_temp_name_buf

[Alan Third]

On Sun, Jul 24, 2016 at 08:28:19PM +0100, Alan Third wrote:
> I think it would make more sense to only check for base64_p when
> HAVE_LONG_FILE_NAMES is defined, though. If it’s not defined, it’s
> currently checking it then executing identical code whatever the
> outcome.
> 
> Once that’s done we’re only duplicating the code once and I don’t
> know if it’s worth introducing a new function for that.

Actually, there’s no reasonable way to do rewrite this so it doesn’t
appear a total of three times, so the function is probably still
useful.
-- 
Alan Third