Re: [Simulavr-devel] 13-pre1

[Theodore A. Roth]

On Thu, 3 Jan 2002, address@hidden wrote:

:)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.

Sorry about that, I got your patches about an hour after I uploaded the 
pre1 patch. Aren't moving targets fun? ;)

:)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.

I've only given your patch a quick review so far. I'll be digging into it 
more once I'm done with email for this morning.

My only concern so far is possible conformance to the gnu makefile
standard as far as what gets installed and where. I'd like to stay close
to that (automake takes care of most of that) but I don't see myself being
too strict about it.

:)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.

I felt that the @param wasn't really needed in this case since signo
should be obvious as to what it does.

In more complicated cases, yes we should use the param tags.

I kinda feel that something like that which should be obvious to even a
casual programmer doesn't really need a description. Someone that needs a 
desciption in an obvious case like these, probably shouldn't be mucking 
around with the code. I'm basing this opinion on Kernighan and Pike's "The 
Practice of Programming" page 23. Of this is only my opinion and history 
has shown that my opinions can be changed. ;)

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

Agreed.

:)
:)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.

Agreed.

I tend to avoid #define constants. Using an enum instead allows you to see 
it's value from within a debugger (with ddd: right click on enum to 
display it, then right click on display and view as hex, dec, etc).

:)
:)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 agreed with you on this, but I'm not as consistent as I should be with 
this. I just tend to like less clutter in the code thus remove the braces 
if they aren't needed. 

:)
:)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.

I think you need the \brief tag unless you enable the JAVADOC_AUTOBRIEF 
option set to on. I would like to use the brief tag in simulavr comments.

:)
:)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.

Agreed. Structures in header files should be documented. Probably should 
document public enums also.

:)
:)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.

Agreed.

:)
:)
:)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;

That one slipped by me. I'll fix it right now.

I'll try to get a pre2 patch out later today.

Ted