Re: set.q

[Ben Pfaff]

John Darrington <address@hidden> writes:

> On Tue, Dec 06, 2005 at 04:13:44PM -0800, Ben Pfaff wrote:
>      
>      Any change to q2c can subtly screw up a lot of other code, and so
>      I'd prefer to limit changes to it to those that actually benefit
>      a lot of other code.  I don't think separating the structures
>      helps anything but this idea for set.q.
>
> I'm not so sure that it wouldn't benefit other code.  When I was
> implementing ONEWAY and EXAMINE , I really wanted to put the output
> routines in a separate module.  But I couldn't.  Everything had to go
> in a single *.q file because that was the scope the cmd_* struct.  My
> proposal would  at least allow the data which pertains to the
> definition of the command and the temporary data used in parsing the
> command to have separate scope and separate modules.
>
> Also, thinking ahead to if and when we implement a client-server PSPP,
> it may be desireable to pass the definition of a command (say struct
> defn_oneway) from the client to the server --- the server is not
> interested how that command was parsed, it just wants to know what to do.

I agree that separating parsing from output is desirable, and it
is the direction that we will want to move in the future.  But I
don't think that moving the sbc_* members into a separate struct
is necessary or sufficient.  The cmd struct has the results of
the q2c-generated parsing code, which is:

        - not in the form you want to describe how to process
          data, because: (1) it doesn't contain everything you
          want, because "custom" parsed info is not in the cmd
          struct; (2) it contains info you don't care about,
          like sbc_* counts and stuff that affects only output,
          not data processing; and (3) data in it is often in the
          wrong form, e.g. you might have to convert an array to
          a bitmap, or merge a set of percentiles and ntiles, or
          whatever (FREQUENCIES does both).

        - not in the form you want to describe how to display
          output, for the same reasons (but with different
          examples).

Basically, it's not really ideal for anything on its own.  It
certainly should not be regarded as a good way to pass data
between modules.  If you want to do that, then it makes sense to
define a structure that has the info you want in it, and *just*
that info, in the proper form for the purpose.  Then copy data
from the cmd struct, or wherever else, into it, and pass the new
struct to that module.  It'll be a lot cleaner than trying to
hack on q2c to do what you want.

The .q files I wrote are lousy examples of software engineering,
so don't take them as gospel.  (I wrote the earliest versions of
them when I was something like 14 years old, give me a break.)
They'll get fixed, they just haven't been yet.

>
>      I don't see why it would take very long to reorganize this way.
>      I'd guess there's about an hour's worth of work in it.
>      
>      Do you really think it's a lot of work?  Why?
>
> It'd be an hour's work if I simply define every subcommand as
> 'custom', and have the custom_* function call the set_* function.  
> But then we'd loose all the range checking etc. that set.q
> currently gives us (things like boxstring=string "x==3 || x==11" "3 or
> 11 characters long" would have to be explicitly implemented).  It
> would also preclude any possibility of implementing intelligent
> command line completion for subcommands.  Re-implementing and testing
> all these sanity checks would take a lot more than an hour. Or can you
> think of another way of doing it? 

> Anyway if you really want to go down the 'custom' path I can do that.

I agree that's a bad way to do it.  But there's no reason to do
it that way.  In the case of BOXSTRING, for example, nothing
needs to change at all: it is parsed only to give an error
message anyhow, because we consider it obsolete.  This is also
true of several other SET subcommands.

Most of the others could be converted simply by replacing
statements of the form "set_name = value;" with "set_name
(value);" and writing a trivial set_name().

SHOW would require a bit more work.  It would probably make sense
to start from the code for it generated by q2c and then hack it
by hand.  Some things don't make sense for us to support in SHOW
anyhow, e.g. as far as I can tell there is no reason to support
SHOWing the various obsolete settings we don't support.

I'm going to check in a big set of clean-ups sometime this week;
I could make this part of it if you like.  Let me know.

> But I think there's merit in separating the descriptions of commands
> from their parsers (I'd like to be able to free the parser data as
> soon as the parser has completed). Perhaps you can think it over for a
> bit. 

I don't think there's much value in freeing parser data early.  I
doubt it takes up a significant amount of memory.
-- 
"Let others praise ancient times; I am glad I was born in these."
--Ovid (43 BC-18 AD)