[ros-dev] top-level source header

[Alex Ionescu]

Casper Hornstrup wrote:

>  
>
>>-----Original Message-----
>>From: ros-dev-bounces at reactos.org [mailto:ros-dev-bounces at reactos.org] On Behalf Of Alex Ionescu
>>Sent: 19. oktober 2005 17:08
>>To: ReactOS Development List
>>Subject: Re: [ros-dev] top-level source header
>>
>>Casper Hornstrup wrote:
>>
>>    
>>
>>>You can lookup the information in the PROGRAMMERS
>>>field in the repository. The PURPOSE field is
>>>not needed as you should be able to get the same
>>>information from the filename and the code in the
>>>file.
>>>
>>>      
>>>
>>I agree it can be considered superflous, but having such identification
>>information is usually considered good practice in most software
>>projects. While -we- can lookup that information, a non-programmer will
>>have NO clue what on Earth a file does simply by browsing through it.
>>    
>>
>[CSH] I'm wondering why it is considered good practice. If a non-programmer
>has no clue as to what the file contents does from reading the source code,
>why does he need to know the information contained in a one line summary
>description of the file contents?
>  
>
I gave an example why below... this question is irrelevant if you had 
read my whole email. I'll say the sample again: documentators.

>As
>  
>
>>for the PROGRAMMERS information, it can be much more useful and correct
>>then looking up anyone that ever touched the file.
>>    
>>
>[CSH] Well history has proved that statement wrong. We have had and still
>have files with headers which were just copied from another file and not
>changed, so it has the wrong information in it. I'm sure you have seen that
>when you worked on ntoskrnl. The repository has correct information when
>the committer was the author and when he isn't, it is usually practised that
>the original author is mentioned in the commit message.
>  
>
You keep avoiding the point: these files might one day find themselves 
OUTSIDE the repository!

>It can also provide
>  
>
>>email information, which SVN doesn't.
>>    
>>
>[CSH] People change e-mail addresses all the time. During this project,
>I've had at least 5 difference e-mail addresses. Repository usernames
>don't change. When you change e-mail address, do you run through all
>the headers and change it? I'm sure many people wouldn't.
>
>We can maintain e-mail addresses at:
>http://www.reactos.org/wiki/index.php/Committers
>then you only need to change it in one central place.
>
>An example: if Foo is browing our
>  
>
>>sources to create some documentation, I'm sure he'd prefer knowing the
>>purpose of each file right out of the bat, and also have contact
>>information for the main developer(s) so he can ask questions.
>>    
>>
>
>[CSH] Tell him to browse using ViewCVS. He can get the username from there.
>  
>
He'll need to go through 30-40 usernames, then figure out which username 
actually wrote useful code. That would take HOURS.

>If he is too lazy to look up the e-mail address of the committer, then we
>can create a website that does it for him. Still way much less work than
>maintaining that information on 10.000+ files in the future. 
>
Funny how every other project can...but we can't? What's to hard about 
adding a purpose to a file? If I can do it, so can other devs (and they 
do, almost everyone here uses a top-level header).

>And when you
>re-arrange the structure of the code (like for instance your ntoskrnl
>changes), the purpose of the files may change and you need to update the
>purpose descriptions. Extra work when developing and history tells us that
>developers forget about such things possible because the compiler doesn't
>warn about it.
>
>  
>
>>>If you can't see the purpose from this, then
>>>the code isn't as clear as it could be.
>>>
>>>      
>>>
>>You're assuming everyone is a developer. You're also assuming every
>>comments their code. There's some stuff in ntoskrnl\mm that I can barely
>>begin to understand; I don't want to imagine what a non-kernel dev, or
>>even a non-dev would glean from it.
>>    
>>
>[CSH] You don't need comments to make code understandable.
>
Nor do you need legs, arms, ears and eyes to live.

> Maybe that code
>could need some refactoring? 
>
If you think refactoring solves the necessity of comments, I'm afraid 
you need to revise that notion. Comments are not meant to explain WHAT 
the code does all the time, but more importantly WHY it does it. When 
you deal with technical code, sometimes you can do things which don't 
make a lot of sense unless they are commented. If you look at the DDK 
sample code, some race conditions were found by Microsoft developers 
after YEARS of testing. And sometimes a line or two of code had to be 
added. If you don't comment it, nobody is going to guess why it's there, 
as many times as you refactor.

Not commenting code is totally bad practice.

>What I would really like to know now is, why
>is a non-programmer reading source code for a piece of highly complex
>software like ReactOS?
>  
>
I've already answered this. Source code is not only for the "elite", you 
know. If only programmers would look at source code, then FOSS wouldn't 
have gained the major popularity it has now. The whole point is that 
even a 10 year old can look at source code and fix something really 
stupid, like:

/* Check if we should do FOO */
if (Foo == FALSE) DoFoo;

>  
>
>>>It's kind
>>>of like documenting your way out of unreadable code.
>>>
>>>
>>>      
>>>
>>Microsoft extensively comments their files yet also includes a "Purpose"
>>field. Almost every software project in the world does.
>>
>>    
>>
>[CSH] Possibly. I couldn't say as I haven't looked at almost every software
>project in the world. I'm still wondering, why do they do it?
>  
>
Look at WINE, Linux, Solaris, Windows...

>  
>
>>>The information in the FILE field is already
>>>displayed in your editor.
>>>
>>>      
>>>
>>The information the FILE field is generally used when a file has been
>>moved from its original location (ie: in forked projects) so that the
>>original may be quickly found. It is invaluable.
>>    
>>
>[CSH] ...if it were kept up to date, which we seem to have a great
>number of problems doing. Why is it invaluable to know the location within
>the repository when it was forked? It may have changed since then.
>  
>
Better then nothing!

>  
>
>>>We should not
>>>misrepresent or not represent copyrights, so that
>>>should be there. We also reuse code from external
>>>projects and it's important to keep the copyright
>>>notices around. The less information there is to
>>>maintain, the more likely it is kept up to date.
>>>
>>>
>>>      
>>>
>>I agree, but stuff like FILE, USAGE and PROGRAMMER don't require that
>>much maintenance.
>>    
>>
>[CSH] Still a lot more than none. There are ~5700 C/C++ files in the
>reactos module. The average space taken up by the FILE, PURPOSE and
>PROGRAMMERS fields the 3 headers below is 183 bytes (without the revision
>changes descriptions). That is 994KB of text for 5700 files or an
>additional 1 minute download time (128Kbit ADSL). And let me just point
>out that ReactOS is still very, very small compared to other mainstream
>Operating Systems. Only a 16MB download at present. 
>
Now that's ironic. When I argued about some build changes which caused a 
~2-4MB reduction in that 16MB I was told that "Who cares about 2-4MB". 
Now we have to care about 994KB?

>Normally I don't care
>much if additional space is used, but this is for information which is
>already present elsewhere.
>
>Let's see some examples.
>
>/*
> * COPYRIGHT:       See COPYING in the top level directory
> * PROJECT:         ReactOS kernel
> * FILE:            ntoskrnl/ke/main.c
> * PURPOSE:         Initalizes the kernel
> *
> * PROGRAMMERS:     Alex Ionescu (cleaned up code, moved Executiv stuff to ex/init.c)
> *                  David Welch (welch at cwcom.net)
> */
>
>So main.c initializes the kernel? Why don't we just rename that file to
>initialize_kernel.c? Or just initialize.c and rename the parent directory
>ke to kernel. Both would give us the same information.
>
>/*
> * COPYRIGHT:       See COPYING in the top level directory
> * PURPOSE:         ReactOS kernel
> * FILE:            ntoskrnl/ke/kqueue.c
> * PURPOSE:         Implement device queues
> *
> * PROGRAMMERS:     Alex Ionescu (alex at relsoft.net) - Several optimizations and implement
> *                                                    usage of Inserted flag + reformat and
> *                                                    add debug output.
> *                  David Welch (welch at mcmail.com)
> */
>
>Device_queue.c anyone?
>
>/* $Id: sem.c 15164 2005-05-09 01:38:29Z sedwards $
> *
> * COPYRIGHT:       See COPYING in the top level directory
> * PROJECT:         ReactOS kernel
> * FILE:            ntoskrnl/ke/sem.c
> * PURPOSE:         Implements kernel semaphores
> *
> * PROGRAMMERS:     David Welch (welch at mcmail.com)
> */
>
>Semaphore.c? But wait, Subversion says Steven changed the file
>last time, yet he's not mentioned in the PROGRAMMERS field.
>Is Subversion lying? Was David the only person making changes
>to this file?
>
>Actually, Subversion tells us that David didn't even write
>half of those lines in the newest revision. A total of 9
>people contributed to the file in the newest revision:
>http://www.csh-consult.dk/sem_c.txt.
>What good is this header information to anyone?
>  
>
NONE. That's why we need to get people to start using it properly (let's 
have a vote). If peopel decide against top-level headers, then fine, I 
won't insist.

>Casper
>
>  
>
Best regards,
Alex Ionescu