Re: [Simulavr-devel] 13-pre1

[reinhard . jessich]

On Thu, 03 Jan 2002, Theodore A. Roth wrote:
> I've just made a 13-pre1 patch with the latest in my cvs tree.
> 
> This should get us all on the same patch again until the repository is 
> move and you all have anon-cvs access.
> 
> I'm putting it on the old web site for now since I won't have to do this 
> in the future.
> 
>   http://res099095.halls.colostate.edu/programs/simulavr/patches/
>     simulavr-0.0.12-0.0.13-pre1.patch.gz

I have tried to apply my doc_2 patch to the package after applying the
13-pre1 patch. This doesn't succeed completely, because you have changed some
things in the meantime in some Makefile.am's.
Please can you intergate my doc_2 patch and upload a new
13-pre2 patch.
I can also provide a new doc_2 patch, but I think it is better you are doing
this, because of the needed patch review.

Until I hear something from you I will work on my scripts.

> I'd especially like some feedback on the new signal stuff.
> 
> The sig.c file has doxygen ready comments. I like the output more than I 
> thought I would since I found all the configuration stuff.

sig.c/.h:

I had a quick look into it and it looks rather good.
You have understood what I mean with a module header. The text is excelent.
It is short and tells me what I need to know about the file.

The function headers are also excelent.

I haven't started doxygen until now. I don't know if it needs a @param for the
function arguments. If it adds the function argument description automaticaly,
then the name of the parameter itself is enough for all functions in that
file. For more complex functions it might be usefull to explain some parameters.

Sometimes, if a function is rather complicated, I add some more comment about
the algorithm of the function.

I haven't seen any comments in the function itself. In this case they are so
simple that this is ok. But if they are more complex, a developer would be happy
to see them to understand the function easier. For example the case statements
in gdb_parse_packet are a good example for necessary comments in a function.
You could omit them, if you use defines instead of the characters, but then
debugging of the protocol with debug outputs is not so easy. It's a trade off.

One suggestion:
I have seen that you omit the braces after an if, if there is only one
statement. In may company we have to use braces in this case, because this
avoids mistakes if you add statements in the future. I have learned that this
is a wise rule in our coding standard. It took me hours to find such errors.

I am not sure if you need the \brief tag. doc++, the father of doxygen, didn't
need it. It used the first words until the '.' for the brief description.

As I explained in one of my previouse mails, I write the comments for the
public functions to the header file. I know then it can be, that the code and
the documentation gets out of sync. This depends how you are working. If you
have the doxygen documentation available, you don't need to look into the
source code of sig.h/.c, if you want to use a function from that file.
But if you don't have it, you will look to the header file and then you will
see nothing.
Normaly I program in C++. There you have to add the doxygen comments to
the header files, because of the class definitions. Maybe it's only a habit
(right word?) of me.
I would suggest to keep the comments in the C file and to add it only to the
header file, if there are structures in it. The resulting documentation must be
well. It doesn't matter from where the documentation is created.

I am sure you have seen writing doxigen comments is simple. You get a good
documentation with only a few cost. It is important to write it immediately when
you write the function. You will appreciate the comments if you read the sig.c
file in 6 months.


gdbserver.c:

In gdb_continue I found a useless
        /* If called from 's', only want to step once */
        if (step == 's')
            break;
this is already handled by
        /* We hit a break point */
        if ( (res == BREAK_POINT) || (step == 's') )
            break;

Haven't tested the code until now, but I am sure it will work.

Regards,
   Reinhard

-- 
 Ing. Reinhard Jessich              mailto: address@hidden
 A-1190 Vienna, Goergengasse 2/2/1  phone: +43/1/3692600
 http://members.telering.at/jessich mobile: +43/664/1735439