From: Alan W. I. <ir...@be...> - 2010-10-07 22:55:44
|
I have mentioned in passing several times on this list that I was planning to change our style from the /* ... */ form of comments to the // form. To review my motivations for this change, it just seems a neater, simpler style to me without all the special leading asterisks that are required to make the multiline /* ... */ form safe to use. I have also recently proved that the // form is fine for our doxygen commentary. Furthermore, // is part of the c99 standard so it is unlikely any modern C compiler will have trouble with this form of comments, and indeed the small number of // form of comments that have been in our C code for a while have not seemed to cause any problems. Previously there have only been two objections (one a concern about doxygen that turned out to be incorrect and one a concern about an old compiler that turned out to be incorrect) to the // style. So I assume there will be no strong objections to this style change. However, on the off-chance there are such strong objections, please let me know tomorrow (Friday) because, otherwise, I plan to make this wholesale change starting this weekend. The planned implementation will consist of an addition to the scripts/style_source.sh script to deal with comment style using the scripts/convert_comment.py python script to do the changes (in --diff or --apply mode) rather than uncrustify (which as far as I know has not implemented this style option yet). As with the rest of the style changes that can potentially be applied by scripts/style_source.sh, the --diff option gives you some safety (if you pay attention!) since it gives you a preview of exactly what is going to be changed for every file before you use the --apply option to actually make those changes. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Alan W. I. <ir...@be...> - 2010-10-11 22:35:07
|
On 2010-10-07 15:55-0700 Alan W. Irwin wrote: > The planned implementation will consist of an addition to the > scripts/style_source.sh script to deal with comment style using the > scripts/convert_comment.py python script to do the changes (in --diff > or --apply mode) rather than uncrustify (which as far as I know has > not implemented this style option yet). As with the rest of the style > changes that can potentially be applied by scripts/style_source.sh, > the --diff option gives you some safety (if you pay attention!) since > it gives you a preview of exactly what is going to be changed for > every file before you use the --apply option to actually make those > changes. I have just done (revision 11257) the first chunk (essentially everything in src) of this commentary conversion project. That generated a ~27000-line context diff! Here are the steps I did including all the checks. I. A preliminary run of the script gave a runtime error because of one embedded comment, e.g., function( /* this is the first argument*/ x ) To my mind this is horrible comment style in any case (and I think everyone else agrees because there was only one instance). I therefore edited the file to get rid of the embedded comment and the check for this issue (anything after the trailing */ on a line generates a runtime error) will continue to be done by the script. N.B. This means new instances of embedded comments will generate script errors, but I doubt our contributors will generate many (if any) embedded comments so I haven't worried about implementing a more graceful way than a run-time error to deal with this case. II. Just out of curiosity I checked src for any non-blank characters in front of // before any changes were applied. software@raven> grep '[^ ]\+.*//' src/*.[ch] |less As expected the small number of instances (mostly URL's inside comments) were legit. III. While looking at the potential changes generated by the script (using "scripts/style_source.sh --diff -au |less") I checked inside the less command for the following two conditions: any trailing blanks; /. $ any insertions that don't have / anywhere on the line (a valid insertion should always have // on the line so if there is no / anywhere, that is an error); /^[+][^/]+$ These searches found nothing. IV. I also eyeballed all the potential changes in the ~27000-line context diff to look for any strange patterns in the resulting // comments. For various special reasons there were three instances of indentation errors for the comments that I spotted. (I have noted these issues and will fix by hand-editing later.) Other than these minor indentation issues I am completely satisfied with the style of the commentary now in src. convert_comment.py is fairly general and hopefully proof against most sorts of wacko styles of comments by our contributors. However, there are no guarantees so the best results will be derived from now on by not straining this script too much. N.B. For example, if you stick with //-style comments from now on, the script is guaranteed to not attempt to change anything in the files. V. The python script is specifically designed not to add or delete any lines to source files. I checked the number of lines in the files before and after the changes were applied to confirm this was the case. VI. After "scripts/style_source.sh --apply" was run, I found I had to run it a second time to take care of some minor realignment that uncrustify wanted to do because of the comment styling changes. But subsequent runs showed no further changes were generated. The reason for this good "non-recursive" result is I have logic in the python script to treat the case of "/***************..." correctly without generating spurious recursions. VII. I tested the new commentary style by running "cmake" and "make -j4 test_noninteractive" and "make -j4 test_interactive" in the build tree. No major build or run-time issues showed up as a result of those tests. Also, the visual results generated by "make -j4 test_interactive" looked good (within the general limitation of how fast they are displayed). N.B. Of course such test results are never complete so keep an eye out during this release cycle for any run-time bugs that may have been introduced by this change in commentary style. ToDo. Go through the same steps for other chunks of our C code. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Alan W. I. <ir...@be...> - 2010-10-22 00:32:40
|
As usual this is taking longer than I had hoped, but I am making some good progress. I just (revision 11269) completed the second chunk of source code (all include/*.h files except for qt.h). On 2010-10-11 15:34-0700 Alan W. Irwin wrote: > Here are the steps I did including all the checks. > > I. A preliminary run of the script gave a runtime error because of > one embedded comment, e.g., > > function( /* this is the first argument*/ x ) > > To my mind this is horrible comment style in any case (and I think > everyone else agrees because there was only one instance). I > therefore edited the file to get rid of the embedded comment and the > check for this issue (anything after the trailing */ on a line > generates a runtime error) will continue to be done by the script. > > N.B. This means new instances of embedded comments will generate > script errors, but I doubt our contributors will generate many (if > any) embedded comments so I haven't worried about implementing a more > graceful way than a run-time error to deal with this case. Actually, in this chunk of C source code I ran into some embedded comments that made sense. So I now leave them unmolested (in the /* ... /* form) in single-line form (i.e., if they are not in the middle of a block comment). If embedded comments do occur by mistake in the middle of a block comment or other comment style shows up which the script doesn't know how to deal with, then scripts/convert_comment.py bails with a runtime error. Furthermore, I have modified scripts/style_source.sh to stop with an overall error message when such runtime errors occur for scripts/convert_comment.py. This new logic worked fine for all files in the latest chunck of source code except for include/plplotcanvas.h which needed some hand editing (revison 11264) to change its pathological comment style into something scripts/convert_comment.py could understand. > > II. Just out of curiosity I checked src for any non-blank characters > in front of // before any changes were applied. > > software@raven> grep '[^ ]\+.*//' src/*.[ch] |less > > As expected the small number of instances (mostly URL's inside comments) > were legit. software@raven> grep '[^ ]\+.*//' $(ls include/*|grep -v qt.h) |less also showed small number of legit instances of //. > > III. While looking at the potential changes generated by the script > (using "scripts/style_source.sh --diff -au |less") I checked inside > the less command for the following two conditions: > > any trailing blanks; > /. $ > > any insertions that don't have / anywhere on the line (a valid insertion > should always have // on the line so if there is no / anywhere, that > is an error); > > /^[+][^/]+$ > > These searches found nothing. Ditto for second chunk. I also searched in the context diff for any line with a combination of "//" and "*" in it with the following two searches: ///.*\* and /\*.*// A fair number of such patterns turned up, but they all looked like mainstream commentary style (or trailing comment style in the second case) which the python script appears to handle with absolutely no problems. > > IV. I also eyeballed all the potential changes in the ~27000-line > context diff to look for any strange patterns in the resulting // > comments. For various special reasons there were three instances of > indentation errors for the comments that I spotted. (I have noted > these issues and will fix by hand-editing later.) Other than these > minor indentation issues I am completely satisfied with the style of > the commentary now in src. > > convert_comment.py is fairly general and hopefully proof against most > sorts of wacko styles of comments by our contributors. However, there > are no guarantees so the best results will be derived from now on by > not straining this script too much. > > N.B. For example, if you stick with //-style comments from now on, the > script is guaranteed to not attempt to change anything in the files. For the second chunk, the context diff that I eyeballed was "only" ~9100 lines. I didn't find any strange patterns in the resulting comments (after the include/plplotcanvas.h case was handled as discussed above). > > V. The python script is specifically designed not to add or delete any > lines to source files. I checked the number of lines in the files > before and after the changes were applied to confirm this was the > case. Same in this case. > > VI. After "scripts/style_source.sh --apply" was run, I found I had to > run it a second time to take care of some minor realignment that > uncrustify wanted to do because of the comment styling changes. But > subsequent runs showed no further changes were generated. Same in this case. > > VII. I tested the new commentary style by running "cmake" and "make > -j4 test_noninteractive" and "make -j4 test_interactive" in the build > tree. No major build or run-time issues showed up as a result of those > tests. Also, the visual results generated by "make -j4 > test_interactive" looked good (within the general limitation of how > fast they are displayed). Same in this case, but instead of doing those commands by hand, I ran software@raven> scripts/comprehensive_test.sh \ --cmake_added_options -DPLD_pdf=ON --build_command 'make -j4' \ --do_nondynamic no --do_static no --do_ctest no \ --do_test_install_tree no --do_test_traditional_install_tree no just to test the extensive header changes in this second chunk as well as to use that updated script in an interesting and useful way. > > N.B. Of course such test results are never complete so keep an eye out > during this release cycle for any run-time bugs that may have been > introduced by this change in commentary style. > > ToDo. Go through the same steps for other chunks of our C code. This second chunk is just a step in the right direction, and there will be more to follow (by removing some of the current temporary limits in scripts/style_source.sh) from e.g., lib, drivers, bindings, and examples, Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Alan W. I. <ir...@be...> - 2010-10-23 17:49:38
|
As of revision 11271 I have converted all C code in the lib subtree. A new issue encountered for this subtree was mixtures of quotes and comments, but I was able to solve that with an update to scripts/convert_comment.py I performed all the tests mentioned before along with a test (also done for already converted source code) to check for bad combinations of '//' and '"'. All tests seemed fine. I think this conversion project is going well with much improved styling of our comments and no nagging script issues, but there is still a lot of source code to convert to the new comment style. Next up for conversion to the new comment style is the C code in the drivers directory. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |