Re: proposal for cvs'ing 'with image'

[Daniel J S. <dan...@ie...>]

Ethan Merritt wrote:

>On Thursday 12 August 2004 02:01 pm, Daniel J Sebald wrote:
>  
>
>>1. Clean up the df_readline() routine for neatness.  If that means a 
>>couple subroutines, that's fine.  But even though that function is 
>>somewhat big, it's flow of logic is pretty straightforward.
>>    
>>
>
>Summarizing my comments of 10-July-2004:
>
>    After I apply the with-image patch, df_readline contains 653 lines of
>    code of which only 303 are shared by the two modes.  
>
>    The largest block of shared code is the section (lines 3362-3465)
>    checking for blank lines, skipped lines, and EOF.  Is this even relevant
>    to binary mode? 
>
>My view of it is that you basically have two separate routines that are
>uneasily co-existing in the same block of code.  I think there should be
>two routines:
>	df_readline      (existing code = 328 lines)
>	df_readbinary  (code path used by pixel/binary routines = ~450 lines)
>By my rough estimate there are at most about 200 lines of code that are
>common to both paths.  I haven't tried to estimate how much of this may
>move easily into a shared subroutine.
>  
>There are only 4 callers of df_readline anyway.  It is the callers 
>business to know whether a binary read or a formatted read is
>appropriate, and call one of the two alternative input routines.
>  
>

Well, breaking up the routine into modules is alright.  But I don't 
agree that a df_readline() and a df_readbinary() is a good idea.  That 
would require a global variable travelling across datafile.c to 
plot2d.c, etc.  Inside of plot2d.c is the following construct

    while ((j = df_readline(v, max_cols)) != DF_EOF) {

How does one easily reconfigure that to incorporate one of two possible 
routines and the global variable to know which is which (df_readline(), 
df_readbinary(), df_general_binary)?  And why is it important for 
plot2d.c to know that it is getting the data from an ASCII or a binary file?

Now, if inside datafile.c you would want something like

int
df_readline(double v[], int max)
{

    if (df_general_binary)
        return df_read_binary_line(v[], max);
    else
        return df_read_ascii_line(v[], max);
}

where df_read_ascii_line() is exactly the same as the current 
df_readline(), that's easy enough.  I would simply copy the existing 
code and separate things so that whatever is currently conditional to 
df_general_binary gets moved to one function and what is left in the 
other.  However, there is some code that would be replicated, but not 
too bad.  Sure, I'm fine with that.  That, in fact would weed out some 
conditionals in exchange for about ten to twenty lines of replicated 
code, so that the total net loss/gain is probably zero.  I opt for 
keeping static variables local to C files, unless well organized.

So, is everyone alright with changing df_readline() as above and calling 
the current df_readline() df_read_ascii_line()?


>  
>
>>>And please check one final time that the patched version of datafile.c
>>>does not inadvertantly back out other changes made since 4.0.
>>>      
>>>
>>How does one check that?
>>    
>>
>
>The changes to datafile.c are so extensive that it is very hard
>to see exactly what is going on.  Can you possibly rearrange
>all the brand new routines so that they are together at the end of the file?
>

Sure.  Easy enough.  Functions down to the bottom of the file... but not 
down below the tables of any of the trm files, right?  And new tables to 
the tops of files... at the bottom of existing tables, or at the top of 
existing tables?

>In fact, ideally I'd like to see a new file containing all the purely new
>routines and also containing the disentangled code from df_readline()
>that constitutes the code path for binary files only.  The original
>df_readline would stay in datafile.c    At that point the remaining
>changes to datafile.c will probably be small enough to understand,
>and much of your new code will be together in one place.
>

What do you want this new file called?  binfile.c?  databin.c?

So, this will be integrated into CVS then?

Dan