Bugzilla – Bug 1237
code cleanups related to includes
Last modified: 2012-09-01 14:58:17 EDT
Creating tracker issue to track this: http://mailman.isi.edu/pipermail/ns-developers/2011-August/009223.html
is this bug dead or obsoleted by the recent bugs 1327-1331?
I guess it's dead like a parrot. My suggestion is to close it.
(In reply to comment #2) > I guess it's dead like a parrot. My suggestion is to close it. I tried to refresh my memory on this; I found in August 2011 archives this note from Vedran: "I propose the following: first local module headers, then ns3/ headers, then C++ headers, then Unix system headers (if any, of course). (This is not what the patch does, but it's already broken by recent changes and should be redone if the consensus is reached.) No idea for general rule on how to sort headers inside those groups (except alphabetically); specifically, per module sorting inside ns3/ headers group is not so easy to do automatically." I do not think it is related to the packaging bugs 1327-1331; perhaps Vedran could comment on the status and whether he would still like to do this. I don't care strongly but I would mention that if we do this, it might be worthwhile to try at the same time to clean up unnecessary includes using deheader if it works for us (http://freecode.com/projects/deheader).
*** Bug 1346 has been marked as a duplicate of this bug. ***
Hi, thanks for adding me to the CC list, I didn't know that this bug existed. (In reply to comment #3) > I do not think it is related to the packaging bugs 1327-1331; perhaps Vedran > could comment on the status and whether he would still like to do this. It's not related to other bugs, it's cleanup stuff you quoted. I will clean up and make changes less invasive (i.e. no reordering of headers, just group in these four groups). This was discussed on the mailing list long time ago and patch was quite big. Two points were discussed: 1) Portability of <cheader> headers; need to test with g++-3.4.6 and g++-4.0. 2) Sorting order of includes: it would be good if we can standardize an order, e.g. first local module headers, then ns3/ headers, then C++ headers and C headers, and finally Unix headers if any. I will post a cleaned-up patch that applies against current codebase in February, as I'm really busy with PhD qualification exam right now. The patch will be smaller than original one, since I will not try to reorder headers inside of the groups mentioned. > I don't care strongly but I would mention that if we do this, it might be > worthwhile to try at the same time to clean up unnecessary includes using > deheader if it works for us (http://freecode.com/projects/deheader). include-what-you-use[1] could also be helpful here, but in that case we should probably first make ns-3 build with clang since it's clang-based. [1] http://code.google.com/p/include-what-you-use/ I will check deheader as well.
Created attachment 1375 [details] Adaptation of dheader for ns-3 This is a python program that evaluates C++ code searching for problems in the import of the headers. To execute it correctly take a look into the waf command line (run waf with -v) in your system and pass it as parameter to the program, or change the make line inside the code. To call the program: python deheader.py [-r (really removes the headers)] -v -m [Compiler used] -w [directory from where the compiler will run] -o [compiler options] The line command I used was: python deheader.py -v -m /usr/lib64/ccache/g++ -w /home/dcamara/INRIA/repos/ns-3-dev -o "-O0 -ggdb -g3 -Wall -Werror -Wno-error=deprecated-declarations -fstrict-aliasing -Wstrict-aliasing -fPIC -pthread -pthread -pthread -Ibuild -I. -I/home/dcamara/INRIA/repos/ns-3-dev/src -I/home/dcamara/INRIA/repos/ns-3-dev/ns3 -I/home/dcamara/INRIA/repos/ns-3-dev/build -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/libxml2 -DNS3_ASSERT_ENABLE -DNS3_LOG_ENABLE -DSQLITE3=1 -DHAVE_IF_TUN_H=1" /home/dcamara/INRIA/repos/ns-3-dev
Created attachment 1376 [details] dhealer final report This file is the output of the dheader adaptation for ns-3 as the code was 10/04 (Changeset = 2a669a0c452e) The code line used was: python deheader.py -v -m /usr/lib64/ccache/g++ -w /home/dcamara/INRIA/repos/ns-3-dev -o "-O0 -ggdb -g3 -Wall -Werror -Wno-error=deprecated-declarations -fstrict-aliasing -Wstrict-aliasing -fPIC -pthread -pthread -pthread -Ibuild -I. -I/home/dcamara/INRIA/repos/ns-3-dev/src -I/home/dcamara/INRIA/repos/ns-3-dev/ns3 -I/home/dcamara/INRIA/repos/ns-3-dev/build -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/libxml2 -DNS3_ASSERT_ENABLE -DNS3_LOG_ENABLE -DSQLITE3=1 -DHAVE_IF_TUN_H=1" /home/dcamara/INRIA/repos/ns-3-dev
Hi Please find in attachment a dhealer modified version for ns-3. With that we can find out that from the 6433 includes of ns-3 code, 2670 are assigned as unnecessary. I have removed the necessary .h, compiled the code with waf and verified that all the tests passed correctly. There are other problems related, for example, with the double include of headers... but that I didn't looked into, sorry. Best regards... Daniel
Daniel, this look interesting. I will try to do the same thing with with iwyu[1] now that ns-3 builds with clang for comparison. [1] http://code.google.com/p/include-what-you-use/
I finally rebased my old patch on top of ns-3-dev and completed it. This one doesn't change order of #include directives, as Mathieu suggested, and covers the entire codebase. To summarize, this patch does the following: * change all <header.h> to <cheader> * remove _all_ occurences of "using namespace std;" and use std:: where appropriate (even most of our examples and tests do it that way, so I changed the rest to that style) * fix bug 1467 by altering FreeBSD version check as suggested in that bug and moving code that defines log2() to core/model/nsmath.h, and using this include where appropriate I left out changing most of <ns3/header.h> to "ns3/header.h" as that would make review harder. My personal opinion is that we should change them for consistency. If we agree that it should be done, it can be automated using sed. If we agree that it shouldn't change, I will revert those that were altered. This compiles with GCC 4.7. I couldn't test it with Clang 3.1 right now, but I doubt any problems will occur. Code is here: http://codereview.appspot.com/6497052 I would be glad to hear about any opinions and concerns you may have before committing this.
(In reply to comment #10) > I finally rebased my old patch on top of ns-3-dev and completed it. This one > doesn't change order of #include directives, as Mathieu suggested, and covers > the entire codebase. > > To summarize, this patch does the following: > * change all <header.h> to <cheader> > * remove _all_ occurences of "using namespace std;" and use std:: where > appropriate (even most of our examples and tests do it that way, so I changed > the rest to that style) > * fix bug 1467 by altering FreeBSD version check as suggested in that bug and > moving code that defines log2() to core/model/nsmath.h, and using this include > where appropriate No need for nsmath.h. We use ns3/math.h so there is no possible ambiguity. > > I left out changing most of <ns3/header.h> to "ns3/header.h" as that would make > review harder. My personal opinion is that we should change them for > consistency. If we agree that it should be done, it can be automated using sed. What is the rationale for that change ? I can't remember any discussion about this, sorry. > If we agree that it shouldn't change, I will revert those that were altered. if we agree that it should be done, it should be done as a separate patch. Would you mind removing these changes from this patch so we can merge it without waiting ? > > This compiles with GCC 4.7. I couldn't test it with Clang 3.1 right now, but I > doubt any problems will occur. > > Code is here: http://codereview.appspot.com/6497052 > > I would be glad to hear about any opinions and concerns you may have before > committing this.
(In reply to comment #11) > No need for nsmath.h. We use ns3/math.h so there is no possible ambiguity. > No problem. Why is then nstime.h used as a name instead of, say, time.h? > What is the rationale for that change ? I can't remember any discussion about > this, sorry. > I can't remember anything beside consistency. > if we agree that it should be done, it should be done as a separate patch. > Would you mind removing these changes from this patch so we can merge it > without waiting ? > Will do today.
Renamed nsmath.h to math.h per Mathieu's suggestion and committed.