On Fri, 21 May 2010 19:04:43 -0400
Doug Manley <doug.man...@gmail.com> wrote:
> Hi, team!
>
> I've submitted numerous patches to the net-snmp project for errors so
> esoteric that it took days to find and understand them.
>
> In my painful searches through the commentless code, I built up an
> understanding of the necessary components and added in many comments
> to help steer people in the right direction, save them hours of time,
> and help everyone extend the code.
>
> I recently realized that a patch that I submitted a year or two ago
> had been stripped of all of its comments (and thus a good deal of its
> usefulness). Granted, my fix (to prevent infinite looping) was kept,
> but all of the knowledge and reasoning behind that fix was stripped.
>
> Here's a piece the original patch:
> @@ -590,10 +591,24 @@
> {
> netsnmp_pdu *newpdu;
>
> - if ((pdu->command != SNMP_MSG_RESPONSE)
> + if (
> + // We're only going to fix a PDU if it was a *response* to us.
> + (pdu->command != SNMP_MSG_RESPONSE)
> + // We're only going to fix a PDU that had an error.
> || (pdu->errstat == SNMP_ERR_NOERROR)
> + // We're only going to fix a PDU that has varbinds.
> || (0 == pdu->variables)
> - || (pdu->errindex <= 0)) {
> + // We're only going to fix a PDU with a positive error-index.
> + || (pdu->errindex <= 0)
> + // We're only going to fix a PDU if we can actually remove
> + // the varbind that was listed as being "bad". If the number
> + // of varbinds is *less* than the error-index value, then
> + // we can't really do anything except simply resend the PDU
> + // and hope for a better response, and that's not a good
> + // thing to do; previously, this had led to infinite loops
> + // of send, receive, and resend.
> + || (pdu->errindex > snmp_varbind_len(pdu))
> + ) {
> return 0; /* pre-condition tests fail */
> }
>
> You'll note that most of the "+" signs are comments. To someone who's
> trying to find the cause of a problem, these are gold; to that person,
> these variables and conditions have no meaning. If you haven't worked
> in the code for long periods of time, a lot of the "obvious" things
> that snmp-gurus might know are completely new and foreign to the rest
> of us.
>
> Maybe someone was against the C++-style comments ("//" comments);
> however, C99 has included them in the C language (see section 6.4.9,
> "Comments", item #2).
>
> In any case, I've seen this kind of behavior in many open-source
> projects, and it only hurts the project. If we cannot justify (to our
> readers, to our developers, to our helpers in the open-source
> community), then we're back to square one of trying to control the
> knowledge about the code.
>
> Unless there is some huge issue of which I am unaware, I strongly
> suggest that everyone fully comment his code; justify its existence,
> and make note of its shortcomings. In the end, we all benefit.
>
> Thank you for your time,
> Doug
>
I am not going to defend the stripping of comments in general.
But most of your example code shows a bad case of the classic
i = 1; // set i to one
In other words, the comment says no more than the code does, and
the comment could be wrong since the compiler interprets code,
not what you think you it does in the comments.
------------------------------------------------------------------------------
_______________________________________________
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
