diff mbox

[3/6] vfprintf: Define WORK_BUFFER_SIZE

Message ID eca7d9307beb107523be4450b90908162943994e.1425246936.git.fweimer@redhat.com
State New
Headers show

Commit Message

Florian Weimer March 1, 2015, 8:52 p.m. UTC 
This constant will allow us to refer to the number of elements in
work_buffer across a function call boundary.
---
 stdio-common/vfprintf.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Paul Eggert March 3, 2015, 2:07 a.m. UTC | #1 
Florian Weimer wrote:
> -  CHAR_T work_buffer[1000];
> +#define WORK_BUFFER_SIZE 1000
> +  CHAR_T work_buffer[WORK_BUFFER_SIZE];

Another nit: I suggest avoiding the macro, as it's confusing when #defined 
inside a function body but intended to be used outside the function, and instead 
doing this at the top level:

enum { WORK_BUFFER_SIZE = 1000 };

The general idea is to use a macro only when necessary.
Florian Weimer March 3, 2015, 7:38 a.m. UTC | #2 
On 03/03/2015 03:07 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>> -  CHAR_T work_buffer[1000];
>> +#define WORK_BUFFER_SIZE 1000
>> +  CHAR_T work_buffer[WORK_BUFFER_SIZE];
> 
> Another nit: I suggest avoiding the macro, as it's confusing when
> #defined inside a function body but intended to be used outside the
> function, and instead doing this at the top level:
> 
> enum { WORK_BUFFER_SIZE = 1000 };
> 
> The general idea is to use a macro only when necessary.

Good idea, thanks.
Carlos O'Donell March 5, 2015, 7:37 p.m. UTC | #3 
On 03/01/2015 03:52 PM, Florian Weimer wrote:
> This constant will allow us to refer to the number of elements in
> work_buffer across a function call boundary.
> ---
>  stdio-common/vfprintf.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Looks to me as long as you follow Paul's suggestion to
use an enum instead of a burried macro.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 16e70b8..dcc24b3 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -235,7 +235,8 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
   const UCHAR_T *end_of_spec;
 
   /* Buffer intermediate results.  */
-  CHAR_T work_buffer[1000];
+#define WORK_BUFFER_SIZE 1000
+  CHAR_T work_buffer[WORK_BUFFER_SIZE];
   CHAR_T *workstart = NULL;
   CHAR_T *workend;
 
@@ -986,7 +987,7 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       /* Print description of error ERRNO.  */				      \
       string =								      \
 	(CHAR_T *) __strerror_r (save_errno, (char *) work_buffer,	      \
-				 sizeof work_buffer);			      \
+				 WORK_BUFFER_SIZE * sizeof (CHAR_T));	      \
       is_long = 0;		/* This is no wide-char string.  */	      \
       goto LABEL (print_string)
 
@@ -1366,7 +1367,7 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       CHAR_T spec;
 
       workstart = NULL;
-      workend = &work_buffer[sizeof (work_buffer) / sizeof (CHAR_T)];
+      workend = work_buffer + WORK_BUFFER_SIZE;
 
       /* Get current character in format string.  */
       JUMP (*++f, step0_jumps);
@@ -1465,7 +1466,7 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	    goto all_done;
 	  }
 
-	if (width >= sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
+	if (width >= WORK_BUFFER_SIZE - 32)
 	  {
 	    /* We have to use a special buffer.  The "32" is just a safe
 	       bet for all the output which is not counted in the width.  */
@@ -1498,7 +1499,7 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	  goto all_done;
 	}
 
-      if (width >= sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
+      if (width >= WORK_BUFFER_SIZE - 32)
 	{
 	  /* We have to use a special buffer.  The "32" is just a safe
 	     bet for all the output which is not counted in the width.  */
@@ -1564,8 +1565,7 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	}
       else
 	prec = 0;
-      if (prec > width
-	  && prec > sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
+      if (prec > width && prec > WORK_BUFFER_SIZE - 32)
 	{
 	  if (__glibc_unlikely (prec >= INT_MAX / sizeof (CHAR_T) - 32))
 	    {
@@ -1935,7 +1935,7 @@  do_positional:
 	CHAR_T spec = specs[nspecs_done].info.spec;
 
 	workstart = NULL;
-	workend = &work_buffer[sizeof (work_buffer) / sizeof (CHAR_T)];
+	workend = work_buffer + WORK_BUFFER_SIZE;
 
 	/* Fill in last information.  */
 	if (specs[nspecs_done].width_arg != -1)
@@ -1969,8 +1969,7 @@  do_positional:
 	  }
 
 	/* Maybe the buffer is too small.  */
-	if (MAX (prec, width) + 32 > (int) (sizeof (work_buffer)
-					    / sizeof (CHAR_T)))
+	if (MAX (prec, width) + 32 > WORK_BUFFER_SIZE)
 	  {
 	    if (__libc_use_alloca ((MAX (prec, width) + 32)
 				   * sizeof (CHAR_T)))