[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[patch #6427] Syntax formatting utilities

From: John Darrington
Subject: [patch #6427] Syntax formatting utilities
Date: Sun, 24 Feb 2008 01:41:02 +0000
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20070505 Iceape/1.0.9 (Debian-1.0.11~pre071022-0etch1)

Follow-up Comment #5, patch #6427 (project pspp):

I don't have any fundamental problems with this, but

1. Let's think of a better prefix.  "gen_string" is so vague, it could mean
almost anything.  How about syntax_fmt_* ?

2. I predict that gen_string will almost always be called with NULL as the
second arg, so why bother with it?  I can see that you wanted to have that arg
for consistency.  But I think it's somewhat inelegent and inefficient to have
most of the occurances passing in null, and then the function testing for null
and doing something different if it's not.  So let's split gen_string into 2
or 3 seperate functions, and make gen_string a wrapper around those 3.

3. In the comments for gen_string: "STR must be encoded ..." What is STR?

4. > int quote; This could be bool ?

5. In gen_pspp_valist, 
+        case 'd':
+          {
+            int i = va_arg (args, int);
+            ds_put_format (output, "%d", i);
+          }
+          break;

This will fail in the general case, eg: "%3d".  We can use gnulib's
printf-parse to help here.  On the other hand perhaps it's not worth doing

6. But at least let's make gen_pspp_valist accept "%%" in the conventional

7.  In gen_{string, number, value etc} I suggest that we make the output
string the first parameter rather than the last.

8.  At the back of my mind, I have the idea that, in order to properly cope
with i18n issues, at some stage in the future, all syntax will have to
internally be converted to wchar_t *.  How would this patch affect that
decision if we took it?


Reply to this item at:


  Message sent via/by Savannah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]