From: sandmann AT clio DOT rice DOT edu (Charles Sandmann)
Message-Id: <10302102358.AA20785@clio.rice.edu>
Subject: Re: fix for copyrite.c[c]/copyrite.pl
To: cyp AT fb14 DOT uni-mainz DOT de (Cyrus Patel)
Date: Mon, 10 Feb 2003 17:58:00 -0600 (CST)
Cc: djgpp-workers AT delorie DOT com (DJGPP developers)
In-Reply-To: <3E47B9E6.29472.C3BDC5DE@localhost> from "Cyrus Patel" at Feb 10, 2003 02:39:14 PM
X-Mailer: ELM [version 2.5 PL2]
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

First - let me say code review shouldn't be taken personally - it's
a way of gathering other opinions to see if it is the best way to
solve a problem.

> Walking the dirs manually instead of glob() adds 48 lines
> - see do_dir() - half of which are white space or brackets.

I would prefer glob() - it works fine, never been a problem,
functionality exists, no need for code duplication. The size
of the name list is small compared to the memory on our 
development machines.  Personal perference.

It's called once a year, so performance is much less an issue
than maintainability.  It's DJGPP specific - so I don't think
portability is very important.  So standard DJGPP and unix type
compilation should be adequate.

> and watcomc too because I was under the (mistaken) impression that 
> the DOS path name length limitation (64 chars) could be worked around 
> if stat()/chdir() etc was done for file/directory names sans path 
> (ie, a simple file/dir name relative to the current dir). I then 
> wanted to find out if there were non-dj libs that could get around 
> it, and if so, how. 

Win32 console build; unix build.

> a) do_file() uses fd io rather than stream io - its more efficient,
>    its easier to check for errors, and its immune to binary/text
>    differences.

I'd prefer we left it as is.

> b) djgpp is in its eighth year. In the 'worst case' scenario, this
>    would mean eight lines of "Copyright (C) 19xx" in a file. This
>    copyright.c coalesces these into one copyright statement (refer 
>    to the copyright.c's comments for a description), and must be 
>    able to parse these too.

I'd prefer we leave all current copyright lines alone and just add
new ones - maybe in the extended format.  I don't like the idea
or complexity of copyright parsing which spans lines.  Overkill.

> d) it scans more than just the first line of a file for non-dj
>    copyrights (just as copyrite.pl does), which, as describes
>    in the first 34 lines of the file, is the whole point of the
>    thing.

This is something I agree we need to do.  But I don't think the code 
needs one time fix stuff it in (such as removing stuff, etc).  It 
will be there forever, dead code ...

> > No documentation (internal or external).

I should clarify - the original program had main(void) - so had no
options - and always behaved a particular way.  The new version 
takes arguments.  If it behaved exactly the same way documentation
still would be desirable - but with no options for a source only,
once a year utility I could understand no docs.

> Anyway, does the number of lines really matter?

Yes and no.  From size on disk, from performance, no.
From long term maintainability, potential bugs, yes.
For code review to see if it's a good safe thing, yes.
If it breaks in 7 years - someone is 6 times less likely to fix it.
The odds of a bug have also increased by about 6.
When Microsoft breaks something (like Win2K) we want a minimum
number of places to fix identical behavior.
If the functionality is necessary, cool.  If not, keep it simple.

> - coalesce multiple dj-(c) lines into one compound 
>   dj-copyright notice per file.

I'd like to avoid this; I didn't check in detail if it does this
when no modification is needed (definitely not desired since this
would cause a revision change on every file in CVS).

> This, as well as the functionality that already existed in 
> copyrite.cc/copyrite.pl, is described in detail at the top of 
> copyrite.c. I'm sorry the copy you got didn't have it. :)

I'm sorry I wasn't clear on my original note, I read those docs
but was referring to usage documentation and one time changes 
versus every time changes.