Re: To fork Net-SNMP over the comments issue? (was: Re: Is it policy to strip comments from the source code that we commit?)
On 24 May 2010 05:11, Omer Zak w...@zak.co.il wrote: 2. Is anyone else in favor of forking the Net-SNMP project over the comments issue? If you wish to fork off a separate project based on the current Net-SNMP code - you are, of course, perfectly free to do so. In many ways, I would welcome such a move - the use of this project (and the consequent volume of support requests) has long grown past the point where the current volunteer team can hope to provide an adequate level of support. It is some time since I've felt able to ensure that every email request receives an adequate answer (and I know that Wes gave up several years before I did). The prospect of moving this burden onto another project is extremely appealing! However, I do feel that you are over-reacting somewhat. I don't know whether you have had a chance to read my response to Doug. But the particular incident in question was more an issue of not _duplicating_ comments, rather than dropping them because they weren't formatted just right. Of course we convert //-style comments to /**/ form where necessary. I can't believe you think we would be so petty as to do anything else! Dave -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: Is it policy to strip comments from the source code that we commit?
Thank you for your time in responding to my question; at our company, we believe in heavily documenting all code before it can be accepted (if that means that people need to work late to get it in, they do ^_^). One of the things that we've found is that what is easy and obvious today is crazy and esoteric tomorrow (especially when it's someone else reading the code). True, this may be a 25-line function, but it's a 25-line function about 25-function-calls into a strange library whose internals I truly have no need or reason to learn or understand (my company uses SNMP to *monitor* data; we do not tweak the library or do any work with it unless we find a problem with it). The fix was conceptually easy, but at the same time, where it was located was hard to find and hard to understand for someone new to the code. Aside: we had once outsourced something to a company who claimed that its code was so good that it essentially was its own documentation (i.e., there were little to no comments). The code itself was fairly easy to follow, but the *intention* behind the code was entirely non-existent. We ended up just scrapping their work and rewriting it from scratch. The point being is that comments usually capture a bit of the intention behind the code; anyone can read valid C (except for some crazy constructs; you know what I mean), but trying to figure out *why* we're checking for less-than-or-equal-to-zero in a particular case is sometimes more important than actually checking for it. By knowing why something was done, one can go deeper both into the code and into understanding it and make greater improvements that would not be possible with just the code itself. I don't mean to criticize anyone in particular; I was hoping that we could collectively make a push for a more coder-friendly source code that newbies can read. Thanks again, Doug On Sun, May 23, 2010 at 6:25 PM, Dave Shield d.t.shi...@liverpool.ac.uk wrote: On 22 May 2010 00:04, Doug Manley doug.man...@gmail.com wrote: 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. As the person who applied that particular patch, I should perhaps say a few words here. Firstly, I would definitely agree that documentation (both comments within the code, and external standalone descriptions) is one of the major weaknesses of the project. There is certainly no general policy of deliberately stripping comments from submitted patches. However, looking again at the patch in question, it effectively involved a one-line change to one relatively short routine (snmp_fix_pdu). This routine is about 25 lines long, and already had a significant comment block at the head, describing the behaviour of that code. My judgement at the time (right or wrong) was that most of the new comments in the patch were simply duplicating descriptions that were already present. I therefore decided to keep the documentation in line with the existing code, and added mention of the new test into that comment block. This also kept the size of the revision relatively small (three lines changed rather than a dozen or so) Looking at the patch again, in the light of this conversation, I'd probably say that there is some useful information which was not already covered. For omitting that, I can only apologise. But I would also stand by my original judgement that there was no benefit in adding in-line comments for each individual test, given the descriptive block immediately above. 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. I can categorically assure you that there is no sense of trying to control knowledge about the code. Both providing support for this project, and my (depressingly rare) attempts to provide new or improved functionality - all has to be fitted into spare time around my main work and home commitments. So anything that improves general understanding (and hopefully reduces the number of questions) is definitely A Good Thing. And I'm sure the other core developers would have a similar view. 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. I would agree 100% with that. The only huge issue is probably the age-old one of time (or lack of it). While I certainly try to comment the code that I write, (and have spent the last twenty years trying to drum the importance of this into our students), it's
Re: Is it policy to strip comments from the source code that we commit?
On Mon, 24 May 2010 18:01:49 -0400, Doug Manley doug.man...@gmail.com said: DM Thank you for your time in responding to my question; at our company, DM we believe in heavily documenting all code before it can be accepted DM (if that means that people need to work late to get it in, they do DM ^_^). One of the things that we've found is that what is easy and DM obvious today is crazy and esoteric tomorrow (especially when it's DM someone else reading the code). One of the fundamental differences between a company and an open source project is that you can't mandate things. The best you can do is not accept the patch unless it's documented. Unfortunately, in this project it seems that would almost always fall down to not accept the patch. Commercial companies tend to like to throw us code randomly over their wall but really don't care that it's not documented nor do they care if we accept it much. We end up taking many scraps from companies because the feature is good and worth-while, even if un-or-poorly-documented. I'm actually in the middle of adding lots of comments to a part of the library that has been before-this largely undocumented. We do what we can when we can. DM I don't mean to criticize anyone in particular; I was hoping that we DM could collectively make a push for a more coder-friendly source code DM that newbies can read. I actually think that you'll only find violent agreement here. Everyone I know that has a desire to spend copious amounts of time on the project for the benefit of it (most notably Dave who has likely spent more than anyone else) agrees that both comments and documentation are extremely important. It's making them exist that has traditionally been difficult. (there is, of course, a fine line as was pointed out by someone else... i++; /* increment i by 1 */ is beyond necessary; every situation has two extremes at least to be avoided) -- Wes Hardaker Please mail all replies to net-snmp-coders@lists.sourceforge.net -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: Is it policy to strip comments from the source code that we commit?
On 22 May 2010 00:04, Doug Manley doug.man...@gmail.com wrote: 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. As the person who applied that particular patch, I should perhaps say a few words here. Firstly, I would definitely agree that documentation (both comments within the code, and external standalone descriptions) is one of the major weaknesses of the project. There is certainly no general policy of deliberately stripping comments from submitted patches. However, looking again at the patch in question, it effectively involved a one-line change to one relatively short routine (snmp_fix_pdu). This routine is about 25 lines long, and already had a significant comment block at the head, describing the behaviour of that code. My judgement at the time (right or wrong) was that most of the new comments in the patch were simply duplicating descriptions that were already present. I therefore decided to keep the documentation in line with the existing code, and added mention of the new test into that comment block. This also kept the size of the revision relatively small (three lines changed rather than a dozen or so) Looking at the patch again, in the light of this conversation, I'd probably say that there is some useful information which was not already covered. For omitting that, I can only apologise. But I would also stand by my original judgement that there was no benefit in adding in-line comments for each individual test, given the descriptive block immediately above. 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. I can categorically assure you that there is no sense of trying to control knowledge about the code. Both providing support for this project, and my (depressingly rare) attempts to provide new or improved functionality - all has to be fitted into spare time around my main work and home commitments. So anything that improves general understanding (and hopefully reduces the number of questions) is definitely A Good Thing. And I'm sure the other core developers would have a similar view. 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. I would agree 100% with that. The only huge issue is probably the age-old one of time (or lack of it). While I certainly try to comment the code that I write, (and have spent the last twenty years trying to drum the importance of this into our students), it's not always easy to judge the best level of description. And sometime we get sloppy, and commit undercommented code. The problem then is finding the time to go back and properly document code that is inadequately covered. If it's a choice between doing that, fixing problems that people have reported, tackling misunderstandings on the lists, or extending functionality - documentation tends to lose out. I wouldn't suggest that this is right, but it's human nature. Anyway, I hope that explains something of why the patch was handled in the way that it was. And even if you don't agree with the decisions that I made, you can at least understand something of the reasoning behind them. Dave -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: To fork Net-SNMP over the comments issue? (was: Re: Is it policy to strip comments from the source code that we commit?)
On Mon, May 24, 2010 at 07:11:02AM +0300, Omer Zak wrote: 2. Is anyone else in favor of forking the Net-SNMP project over the comments issue? Forking a major project over *comments*? -- Stephen J Friedl | Security Consultant | UNIX Wizard | 714 694-0494 st...@unixwiz.net | Orange County, CA | Microsoft MVP | unixwiz.net -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
To fork Net-SNMP over the comments issue? (was: Re: Is it policy to strip comments from the source code that we commit?)
In view of the recent discussion about stripping comments off patches: 1. One of the issues is the fact that comments starting with // (rather than enclosed by /* . . . */) are stripped off, rather than converted from the first into the second. The reason behind this is valid, although I doubt whether nowadays there is any C compiler not supporting the // notation. 2. Is anyone else in favor of forking the Net-SNMP project over the comments issue? The forked version would follow the same policies as Net-SNMP except that all comments are retained. Any // comments would be converted into /* . . . */ comments by a commit script. Rationale: I expect it to be easier to fix wrong comments than to document and add comments to code lacking them. --- Omer -- Philip Machanick: caution: if you write code like this, immediately after you are fired the person assigned to maintaining your code after you leave will resign My own blog is at http://www.zak.co.il/tddpirate/ My opinions, as expressed in this E-mail message, are mine alone. They do not represent the official policy of any organization with which I may be affiliated in any way. WARNING TO SPAMMERS: at http://www.zak.co.il/spamwarning.html -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: To fork Net-SNMP over the comments issue? (was: Re: Is it policy to strip comments from the source code that we commit?)
On Sun, 2010-05-23 at 21:14 -0700, Steve Friedl wrote: On Mon, May 24, 2010 at 07:11:02AM +0300, Omer Zak wrote: 2. Is anyone else in favor of forking the Net-SNMP project over the comments issue? Forking a major project over *comments*? Yes. The comments situation in Net-SNMP is grave. I already wasted lot of time trying to understand Net-SNMP agent code due to insufficient comments. --- Omer -- Philip Machanick: caution: if you write code like this, immediately after you are fired the person assigned to maintaining your code after you leave will resign My own blog is at http://www.zak.co.il/tddpirate/ My opinions, as expressed in this E-mail message, are mine alone. They do not represent the official policy of any organization with which I may be affiliated in any way. WARNING TO SPAMMERS: at http://www.zak.co.il/spamwarning.html -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: Is it policy to strip comments from the source code that we commit?
Omer Zakwrote wrote: Now I am flabbergasted that anything leading toward such documentation has been censored from Net-SNMP commits. Like Wes said: we don't censor useful, properly formatted comments. +Thomas -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: Is it policy to strip comments from the source code that we commit?
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
Re: Is it policy to strip comments from the source code that we commit?
On Fri, 21 May 2010 19:04:43 -0400, Doug Manley doug.man...@gmail.com said: DM I've submitted numerous patches to the net-snmp project for errors so DM esoteric that it took days to find and understand them. I'd have to go back and chase the details to figure out who committed the patch, why it might have been edited, etc. If you provide a patch number I can look into it. However, comments are generally supposed to be left in with some exceptions: 1) they're not useful or are hard to understand (possibly harder than the code). 2) they're improperly formatted (though generally I'd encourage our developers to fix the formatting rather than remove them entirely). //-style comments are not acceptable in the package. We try and support as many compilers as possible, including those that aren't fully C99 compliant. And it doesn't hurt to have /* style comments instead (and for what it's worth, I'd rather use // myself but restrict my own code to /* style comments). In fact, we have a pre-release check that specifically looks for // comments and flags them as a problem. -- Wes Hardaker Please mail all replies to net-snmp-coders@lists.sourceforge.net -- ___ Net-snmp-coders mailing list Net-snmp-coders@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
Re: Is it policy to strip comments from the source code that we commit?
Oh, this explains why I was forced to spend days (elapsed time weeks)
understanding the Net-SNMP agent code, which I need to modify in order
to implement an audit log feature.
AARRRH!!
A while ago, I posted to the Net-SNMP users mailing list a question
about a guide to the source code, or a design document, which explains
the higher level organization of the code and its flow of control. No
one answered me.
Now I am flabbergasted that anything leading toward such documentation
has been censored from Net-SNMP commits.
Contrast this policy with the Linux kernel one of retaining comments and
even having a Documentation subdirectory.
--- Omer
On Fri, 2010-05-21 at 19:04 -0400, Doug Manley 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
--
In civilized societies, captions are as important in movies as
soundtracks, professional photography and expert editing.
My own blog is at http://www.zak.co.il/tddpirate/
My opinions, as expressed in this E-mail message, are mine alone.
They do not represent the official policy of any organization with which
I may be affiliated in any way.
WARNING TO SPAMMERS: at http://www.zak.co.il/spamwarning.html
--
___
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
