patchPSPP - Patches: patch #6427, Syntax formatting utilities

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6427: Syntax formatting utilities

Submitter:  Ben Pfaff <blp>
Submitted:  Wed 20 Feb 2008 07:04:02 AM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed

Jump to the original submission

Wed 05 Mar 2008 06:31:02 AM UTC, comment #8: 


>This looks fine now. I haven't actually tried it.


Thanks!  I checked this in.

Ben Pfaff <blp>
Group administrator
Mon 03 Mar 2008 10:48:53 AM UTC, comment #7: 

This looks fine now.   I haven't actually tried it.

John Darrington <jmd>
Group administrator
Mon 03 Mar 2008 05:30:22 AM UTC, comment #6: 

Thanks for the comments.  I'm posting a revised version now.

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


How about syntax_gen_*?  We have enough references to data "formats" already without adding a new one.

>2. I predict that gen_string will almost always be called with
>NULL as the second arg, so why bother with it?


OK.  I deleted that argument.

>So let's split gen_string into 2 or 3 seperate functions, and make
>gen_string a wrapper around those 3.


Let's just use the single function for now, and if there's a good reason to use a non-null argument we can invent a new function for later.

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


That's what I get for copying your comments without properly editing them.  Fixed.

>4. > int quote; This could be bool ?


Actually, no, the value of quote is used in the loop.

>5. In gen_pspp_valist, ...This will fail in the general case,
>eg: "%3d".


That function is supposed to be specialized for generating PSPP syntax, and I can envision little reason to want to create arbitrary field widths, etc., inside PSPP syntax.

I can forsee wanting to use a variety of different types in these format strings, e.g. "%ld", "%zu", and so on.  But I'm not certain yet that these functions are actually going to be useful enough to carefully implement all these types yet.  I'm happy to implement them if needed later, just not before it's demonstrably a good function in general.

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


Sure.  Fixed.

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


OK.  Fixed.

>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?


Possibly it could make it easier, because it would group much of the syntax generation code into a single file would be easier to change than if it were scattered throughout the source code.  I don't have any more detailed predictions than that; I've never conducted such a transition before.

(file #15160)

Ben Pfaff <blp>
Group administrator
Sun 24 Feb 2008 01:41:01 AM UTC, comment #5: 

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 yet.

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

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?

John Darrington <jmd>
Group administrator
Sun 24 Feb 2008 12:18:44 AM UTC, comment #4: 

sigh

(file #15101)

Ben Pfaff <blp>
Group administrator
Sun 24 Feb 2008 12:16:48 AM UTC, comment #3: 

Here's a reworked version of this patch, which incorporates libpspp/syntax-gen.[ch] and moves both into src/ui.

John, I hope that the change in semantics from gen_quoted_string() to gen_string() is OK with you.  I think that the gen_string() version is more generally useful than the in-place quoting of a string that gen_quoted_string() would do.

Let me know what you think.

Ben Pfaff <blp>
Group administrator
Wed 20 Feb 2008 02:59:46 PM UTC, comment #2: 

I'd completely missed syntax-gen.[ch].  I'm glad that there isn't too much overlap in functionality.  I'll rework the patch to incorporate syntax-gen.[ch] and move both into src/ui.

Ben Pfaff <blp>
Group administrator
Wed 20 Feb 2008 12:00:57 PM UTC, comment #1: 

The need for some kind of framework for this purpose is becoming apparent.

We already have libpspp/syntax-gen.[ch] so I think it should go in the same directory at least.  Arguably ./libpspp is too low a level.  Conversely ./ui/gui is too high. Perhaps these things should go in ./ui

John Darrington <jmd>
Group administrator
Wed 20 Feb 2008 07:04:02 AM UTC, original submission:  

It's nontrivial to get output PSPP syntax both syntactically correct and easy for users to read.  This patch includes a number of useful functions for composing PSPP syntax.  It will be used by the text-import patch, but I also hope that it will be useful in many other parts of the GUI for generating syntax.

What do you think?

Ben Pfaff <blp>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #15160:  format-syntax.patch added by blp (22KiB - text/x-diff)
file #15101:  format-syntax.patch added by blp (23KiB - text/x-diff)
file #15080:  format-syntax.patch added by blp (12KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jmd (Posted a comment)
  • -email is unavailable- added by blp (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-03-05 blp StatusReady For Test/Review Done
        Open/ClosedOpen Closed
    2008-03-03 blp Attached File- Added format-syntax.patch, #15160
    2008-02-24 blp Attached File- Added format-syntax.patch, #15101
    2008-02-20 blp Attached File- Added format-syntax.patch, #15080

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code