Bugzilla – Bug 938
missing Doxygen in ns-3
Last modified: 2024-03-27 20:31:48 EDT
Created attachment 914 [details] list of missing doxygen Attached file shows current state of missing Doxygen for ns-3. We are proposing to block ns-3.9 until clearing this backlog of missing doxygen. Also, we should revisit this doxygen.conf file which hasn't been touched for a while; the EXCLUDEs may be stale EXCLUDE = src/routing/olsr/olsr-state.h \ src/routing/olsr/olsr-repositories.h \ src/simulator/high-precision.h \ src/simulator/high-precision-128.h \ src/simulator/high-precision-double
Thank you Pavel and others who have started this cleanup. I decided not to block the release on this but we'd like to aim for full cleanup by ns-3.10.
Update: I would like to make progress on this as follows: 1) fix errors on a module-by-module basis, starting with core and working outward in the dependency tree. Specifically, disable these two warnings in doxygen.conf about undocumented components and fix just the remaining errors: -WARN_IF_UNDOCUMENTED = YES +WARN_IF_UNDOCUMENTED = NO -WARN_NO_PARAMDOC = YES +WARN_NO_PARAMDOC = NO 2) once ns-3-dev is clean from step 1, start buildbot testing of doxygen in this modified configuration to prevent new errors. Also start daily reporting of the count of undocumented components (i.e. with the two above flags enabled) so that we can track that the number does not grow (however, don't report a failed test for these undocumented items). 3) start working on undocumented components starting with module core and moving outward. I think that 1) and 2) are reasonable goals for the ns-3.12
Hi Tom, is there a patch on this already? Back in June, some of my students and me have been working on cleaning ns-3 Doxygen documentation and made a lot of progress on 2). We have a work-in-progress patch that I will post here once it's fixed to apply cleanly to ns-3-dev. If there are no objections, I'm willing to take this bug.
(In reply to comment #3) > Hi Tom, is there a patch on this already? No > > Back in June, some of my students and me have been working on cleaning ns-3 > Doxygen documentation and made a lot of progress on 2). We have a > work-in-progress patch that I will post here once it's fixed to apply cleanly > to ns-3-dev. If there are no objections, I'm willing to take this bug. That would be much appreciated. Please don't hesitate to post partial fixes as well.
Hi Tom, do we prefer \ to @ for doxygen commands, and do we prefer /** */ format for long docs to multiple ///? I have a student interested in helping with this; will revisit doxygen.conf msyself; patch soon.
(In reply to comment #5) This is addressed in the coding style document: http://www.nsnam.org/developers/contributing-code/coding-style/#comments > do we prefer \ to @ for doxygen commands, and structural commands may start either with \ or @; both styles are found in the existing codebase. > do we prefer /** */ format for long docs to multiple ///? Also for this question, it has also been done both ways, but I believe that the /** */ is found more than the /// style. In general, since this is an effort to add missing doxygen, I would suggest to keep with prevailing style for each file. Or, try to determine the prevailing style for the module as a whole (if there is one) and use that. Note, if you have suggestions to make the coding style document more clear in this regard, feel free to suggest that too.
Created attachment 1265 [details] Work-in-progress patch As promised, I ported the work we did on this before summer to latest ns-3-dev. This fixes some doxgen errors we encountered back then, but undocumented stuff is mostly unchanged. Patch also replaces all @commands stuff with \command for consistency. I plan to resolve all undefined references and references to functions that don't exist, as there aren't that many: $ grep -v "not documented" doxygen.log | grep -v "not (all) documented" | wc -l 111 Some feedback is appreciated.
As for fixing undocumented stuff: I would like to fix all other warnings first, so that only "undocumented" warnings remain, and then perhaps interested students over here can work on documenting stuff that is missing. I could even organize a documentation marathon event, but that will not happen soon enough for 3.13; 3.14 or 3.15 is much more realistic target. P.S. I forgot to take a look at EXCLUDE = stuff; will do.
Created attachment 1266 [details] Doxygen fixes (updated patch) This fixes all but undocumented stuff and undefined references.
The patch is... well... huge. It's also a bit hard to separate what's been changed for consistency (@ with \) and what to fix errors. Normally I'd ask to separate the patch in two, one to just fix errors and the other one to do the consistency changes, but nvm, separating them now would be a real pain, and not really worth. Plus, we're talking about broken documentation, so what could go bad, some more documentation broken? No. Some more errors? No. Go ahead. +1 PS: yes, I checked the patch, just not all the lines one by one.
Thanks for review Tommaso. Missing documentation patches will be separated per module and per person who did them. We plan to start with core this week.
OK, we started with core, but this is a _lot_ of work. There is over 900 undocumented functions just in core. (Around 400 in network.) Me and my students can deal with those two in a month or two (it's not trivial; we need to check what a function does and document that). If we want to get the whole ns-3 documented anytime soon, we need more people working on this. Tom, is the patch acceptable for checking in? Is there anything I else need to fix?
(In reply to comment #12) > > Tom, is the patch acceptable for checking in? Is there anything I else need to > fix? I scanned the patch; it has some fixes but seems to be mostly style changes (changing @ to \). I would hesitate to check this whole patch in as-is without some buy in from the authors and maintainer about making purely style changes.
Tommaso, I think a have a way to split it that isn't painful. Tom, I will split out the real changes from style changes. Does that mean that parts that fix errors are OK? Also, if we document core and in the process we also fix style consistency and move some docs around files (e. g. from .cc to .h) and groups, that is OK because it's not purely style changes and because it's all in a single module?
NACK to gigantic stylistic changes. This is one too many. I have tried to review the patch but I do not have enough bandwidth to filter out the non-stylistic changes from the actually useful content.
Thanks for the review Mathieu. Will attach a no-style-fixes patch in a minute.
Created attachment 1267 [details] Doxygen patch that fixes erros without style changes
Wasn't a minute exactly, but the idea worked. Hope this is good.
Created attachment 1268 [details] Topology-reader docs fixing
Please find in the latest attachment some improvement to the topology-reader module docs. 1) fixed 2 warnings about missing documentation 2) added a .rst description for the manual (hope it works, I can't compile it right now 'cuz Lion broke dia). 3) changed a bit the documentation so the classes do show up in the module list. (added \ingroup topology to all the classes). I'd be happy if you could check if the manual renders properly and, in case, push it on the main repository. Cheers, Tommaso
Created attachment 1269 [details] Topology docs fixings Found the way to compile the manual, fixed the pdf layout and more.
I think we should push Vedran's patch (at least the one to fix the bugs) before 3.13. Also, mind if I push the TopologyReader one since I'm the maintainer ? Anyone had the time to check it ? Cheers, Tommaso
(In reply to comment #22) > I think we should push Vedran's patch (at least the one to fix the bugs) before > 3.13. Agreed. I asked Mitch to have a look at committing this, among a couple of other doxygen-related things. > > Also, mind if I push the TopologyReader one since I'm the maintainer ? Anyone > had the time to check it ? Please push it, unless you really would like to wait for someone to review (in which case, let me know).
(In reply to comment #18) > Wasn't a minute exactly, but the idea worked. > > Hope this is good. +1 if you checked that the style checker does not undo some of your changes after you run it on top of your patch
Comment on attachment 1269 [details] Topology docs fixings Applied the patches to fix TopologyRead module
The patch modifies the doxygen configuration file so that doxygen will scan the example directories in each module. Is that what people want? Here is the description for the doxygen tag that controls this behavior: The EXAMPLE_PATH tag can be used to specify one or more files or directories that contain example code fragments that are included (see the \include command). Before the patch: EXAMPLE_PATH = After the patch: EXAMPLE_PATH = src/aodv/examples \ src/bridge/examples \ src/click/examples \ src/config-store/examples \ src/core/examples \ src/csma/examples \ src/csma-layout/examples \ src/dsdv/examples \ src/emu/examples \ src/energy/examples \ src/flow-monitor/examples \ src/internet/examples \ src/lte/examples \ src/mesh/examples \ src/mobility/examples \ src/mpi/examples \ src/netanim/examples \ src/network/examples \ src/nix-vector-routing/examples \ src/olsr/examples \ src/openflow/examples \ src/point-to-point/examples \ src/propagation/examples \ src/spectrum/examples \ src/tap-bridge/examples \ src/template/examples \ src/tools/examples \ src/topology-read/examples \ src/uan/examples \ src/virtual-net-device/examples \ src/visualizer/examples \ src/wifi/examples \ src/wimax/examples
IMHO yes; files src/core/model/callback.h and src/core/model/simulator.h already include examples, and I believe that as we progress with Doxygen cleanup there will be more examples included.
I don't think that the stylistic changes I found outweigh the value of the patch. But, there are many changes in the patch that might be considered stylistic. For example, the patch adds spaces before \{ and \} tags: diff -r 159633363724 src/mesh/model/dot11s/peer-management-protocol.h --- a/src/mesh/model/dot11s/peer-management-protocol.h Mon Oct 31 12:13:18 2011 +0000 +++ b/src/mesh/model/dot11s/peer-management-protocol.h Fri Nov 04 18:06:33 2011 +0100 @@ -82,7 +82,7 @@ * \param beaconTiming beacon timing element (needed by BCA) */ void ReceiveBeacon (uint32_t interface, Mac48Address peerAddress, Time beaconInterval, Ptr<IeBeaconTiming> beaconTiming); - //\} + // \} /** * \brief Methods that handle Peer link management frames * interaction: @@ -125,9 +125,9 @@ * \brief Checks if there is established link */ bool IsActiveLink (uint32_t interface, Mac48Address peerAddress); - //\} + // \} ///\name Interface to other protocols (MLME) - //\{ + // \{ The patch also replaces some some / characters with space characters that appear before a \{ or \} tag: diff -r 159633363724 src/mesh/model/dot11s/peer-management-protocol-mac.h --- a/src/mesh/model/dot11s/peer-management-protocol-mac.h Mon Oct 31 12:13:18 2011 +0000 +++ b/src/mesh/model/dot11s/peer-management-protocol-mac.h Fri Nov 04 18:06:33 2011 +0100 @@ -45,26 +45,30 @@ PeerManagementProtocolMac (uint32_t interface, Ptr<PeerManagementProtocol> protocol); ~PeerManagementProtocolMac (); ///\name Inherited from plugin abstract class - ///\{ + // \{ void SetParent (Ptr<MeshWifiInterfaceMac> parent); bool Receive (Ptr<Packet> packet, const WifiMacHeader & header); bool UpdateOutcomingFrame (Ptr<Packet> packet, WifiMacHeader & header, Mac48Address from, Mac48Address to); void UpdateBeacon (MeshWifiBeacon & beacon) const; - ///\} - ///\name Statistics: - ///\{ + // \} + ///\name Statistics + // \{ void Report (std::ostream &) const; void ResetStats (); uint32_t GetLinkMetric (Mac48Address peerAddress); - ///\} + // \}
(In reply to comment #28) > I don't think that the stylistic changes I found outweigh the value of the > patch. > > But, there are many changes in the patch that might be considered stylistic. > > For example, the patch adds spaces before \{ and \} tags: I'm not sure whether it is purely stylistic; \\\ is for single-line comments, while these lines have curly braces to start multi-line grouping. However, I agree that at this point, I would just as soon commit it even if there were a few whitespace changes.
Thanks for the review Mitch. If I recall correctly, Doxygen even complained about ///\{ and wanted // \{ instead (i.e. 2 slashes instead of 3). What I'm certain, however, is that this is the way of writing it per Doxygen manual. Thanks Tom. Hope the commit happens soon.
Should I commit the patch?
(In reply to comment #30) > Thanks for the review Mitch. If I recall correctly, Doxygen even complained > about ///\{ and wanted // \{ instead (i.e. 2 slashes instead of 3). What I'm > certain, however, is that this is the way of writing it per Doxygen manual. > > Thanks Tom. Hope the commit happens soon. pushed your latest patch in changeset 57ba46094a0d
Thanks for the commit.
Created attachment 1712 [details] MPI modules patch for Doxygen warnings Only noted two public APIs that required attention.
Created attachment 1713 [details] current list of doxygen warnings to fix
Created attachment 1714 [details] the actual warnings part 1 of actual warnings
Created attachment 1715 [details] actual warnings (part 2)
Created attachment 1716 [details] Stats module patch for Doxygen warnings Patches from both the model and helper directories of the stats module. Confirmed all changes within patch file, and confirmed rebuild of ns-3 including patches.
Created attachment 1717 [details] Patch to partially fix doxygen warnings in wifi module From ns-3 Sprints on 11/15/2013
Created attachment 1718 [details] Patch to fix some doxygen warnings in lte module From the ns-3 documentation sprint November 15, 2013. Nicola, I add you in CC list, in case you want to review this first. -budi-
(In reply to Budiarto Herman from comment #40) > Created attachment 1718 [details] > Patch to fix some doxygen warnings in lte module +1 for me, thanks for the good work!
Created attachment 1719 [details] Internet model fixes This patch fixes all the doxygen warnings in internet/model/ (Internet/helper next). Please review the NSC related fixes, as I'm not fluent with NSC and some descriptions might be wrong. Ty.
Created attachment 1720 [details] New Internet docs patch This new patch eliminates some zombie functions (defined but not implemented) and fixes also the helpers warnings.
Created attachment 1721 [details] flow-monitor model + helper This patch fixes all the Doxygen warnings in the flow-monitor module (except for examples and tests)
Created attachment 1724 [details] log of current doxygen warnings
Created attachment 1725 [details] summary of current warnings report
Created attachment 1726 [details] summary of current warnings
Created attachment 1727 [details] A second patch to partially fix doxygen warnings in wifi module More fix for wifi module
Added a wiki page to track who's working on what. http://www.nsnam.org/wiki/Doxygen-warnings
Created attachment 1737 [details] stats Doxygen fixes Posting the patch and not directly applying because I changed a lot the "stats" Doxygen section.
(In reply to Tommaso Pecorella from comment #50) > Created attachment 1737 [details] > stats Doxygen fixes > > Posting the patch and not directly applying because I changed a lot the > "stats" Doxygen section. I am fine with the changes, except for one statement below, so I suggest applying now. "Probes are a special kind of Data Aggregator" should read instead: "Probes are a special kind of Trace Source"
Comment on attachment 1737 [details] stats Doxygen fixes Pushed
Created attachment 1790 [details] Network module fixes Network module fixed. Well, not the test and examples, but everything else is fixed. It might be needed to re-check the \ingroup things, but at least all is documented. Feel free to check the content...
The network module fixes contains a spurious optimisation to address.cc's static uint8_t AsInt (std::string v) The same function is used in other places, but like I said in a mail, that function is basically useless (it can be replaced with a single standard library call, and it's 10x faster too). I'd remove all the AsInt functions to be honest. However, probably it's best to do it in a separate patch.
(In reply to Tommaso Pecorella from comment #53) > Created attachment 1790 [details] > Network module fixes > > Network module fixed. Well, not the test and examples, but everything else > is fixed. > > It might be needed to re-check the \ingroup things, but at least all is > documented. > > Feel free to check the content... A lot of very good work here. I particularly like RED queue stuff. * ByteTagList::Iterator BeginAll (void) const Shouldn't you have \brief Returns an iterator pointing to the very first tag in this list.? * ItemIterator, PacketMetadata, Item, ByteTagIterator, PacketTagIterator, Packet constructors, same as above * Just a minor thing: I would prefer changes to * strtoul() * noBUFFER_FREE_LIST * friend bool operator < (Ipv4Address const &a, Ipv4Address const &b) to be a separate patch. Otherwise +1.
Hi, the minor things you pointed out have been pushed in a previous patch (strtoul), or are typos (noBUFFER_FREE_LIST). The friend operator is mostly cosmetic, as it's helping the docs to be in the right place. I've improved the doc a bit about the other things you mentioned. I guess the docs can be refined further at a later stage. T.
Comment on attachment 1790 [details] Network module fixes Pushed in changeset 10657:6531a8817def
Created attachment 1890 [details] Point-to-point documentation patch Fixes all warning into the module
Created attachment 1891 [details] Internet module fixes
Created attachment 1892 [details] mobility patch from Tiago
Created attachment 1893 [details] propagation models patch This patch also removes some unused functions, variables and custom PI definitions.
Created attachment 1896 [details] Patch to fix some doxygen warnings in lte module From the ns-3 documentation sprint October 3, 2014.
Created attachment 1897 [details] Asn1 header docs from Tommaso
(In reply to Peter Barnes from comment #63) > Created attachment 1897 [details] > Asn1 header docs from Tommaso Wow, I should gripe about more files if this is the result :) (thanks) I will plan to apply all of the pending patches today.
*** Bug 766 has been marked as a duplicate of this bug. ***
Created attachment 1982 [details] Sphinx docs for propagation module The propagation model sphinx docs is still (almost) empty. This patch "only" adapts the doxygen models descriptions and places then also in the .rst. In my opinion it's good to have the descriptions in both places: it saves a lot of time. There are two models still to document/clarify. One is the JakesPropagationLossModel, and I asked Kirill to provide some more infos, the other one is the Cost231PropagationLossModel. About the Cost231, I guess that the manual should be amended, as there's a lot of confusion: the Okamura-Hata model it referring to the Cost231 formulas, and the Cost231 model description is absent. I think that we must clarify the models relationship and how they are different.
Created attachment 2040 [details] some small fixes Some small fixes, dunno why but they're not caught by the normal logs. It's not exhaustive, these are just the ones I found.
Created attachment 2041 [details] Internet module fixes Not applying directly because it contains a function removed. It is a static function that is only declared. It is not implemented and removing it seems to be safe (no compilation errors). Why it was there... no idea. Once upon a time it was marked as "internal". I guess it was removed at some point.
(In reply to Tommaso Pecorella from comment #68) > Created attachment 2041 [details] > Internet module fixes > > Not applying directly because it contains a function removed. > It is a static function that is only declared. It is not implemented and > removing it seems to be safe (no compilation errors). > Why it was there... no idea. Once upon a time it was marked as "internal". I > guess it was removed at some point. I think it is safe to remove
Created attachment 2043 [details] Spectrum model stuff Not just documentation...
Created attachment 2120 [details] updated patch for the Spectrum model
I updated the table here: https://www.nsnam.org/wiki/Doxygen-warnings#Doxygen_work_status This excludes tests and examples. The total is 9237 warnings, so great progress. Are we sure we're ready for last call? Or do we still want to push progress on doxygen?
The last call state was in reference to the two lingering patches that were from earlier this year, from Tommaso, which I just pushed. I would like to keep fixing these, so I moved the state back to Patch Wanted.
Created attachment 2249 [details] OLSR documentation update.
Created attachment 2250 [details] OLSR uncrustified To be applied on top of patch 2249. Perhaps it's a bit too zealous, but if one is at it...
Comment on attachment 2249 [details] OLSR documentation update. changeset 12025:e56715abf931
File updates to correct doxygen errors uploaded as http://codereview.appspot.com/313230043
In reference to https://www.nsnam.org/wiki/Doxygen-warnings#Doxygen_work_status, and the use of doxygen.warnings.report.sh, I have submitted an enhancement (Bug 2592) to doxygen.warnings.report.sh to add an -S option to sort the log file alphabetically by file name (-s was already taken).
I have uploaded modified files to resolve all DOXYGEN warnings (with a few exceptions, primarily python files) for all SRC folders (except LTE which still has thousands of warnings for just that model) for the non Test and Example subfolders. The uploaded changes also resolve all DOXYGEN warnings for all Test and Example subfolders for the SRC folders Antenna through Mesh, excluding LTE. The latest patch set is a comprehensive set of files. The remaining warnings will be resolved over the next week and uploaded as a new patch set, however, due to the large number of files modified, I would like to see integration of the already modified files into the baseline started so all of the modifications can be in place for the NS 3.27 release. Please ignore the file doc/doxygen.warnings.report.sh which was uploaded in error.
+1 for committing Robert's work at http://codereview.appspot.com/313230043 I'll add some notes for follow up in a subsequent comment here.
(In reply to Peter Barnes from comment #80) > +1 for committing Robert's work at http://codereview.appspot.com/313230043 > > I'll add some notes for follow up in a subsequent comment here. I'm honestly impressed by Robert's work, but I'd not commit it as it is. Some fixes are simply perfect, some others are... not so perfect. Examples: src/wave/model/channel-scheduler.h - the 3 initializers are not explained, and in 2 o them the params are not documented. - the ChannelAccess enumeration elements are not documented. src/wimax/model/wimax-tlv.h - Serialize, Deserialize and GetSerializedSize shouldn't be documented (they are virtual, the documentation is inherited from the base class) - CommonTypes enumeration elements are not documented - "/// constructor" is redundant (personal opinion), but when it is documented, the params must be documented as well. - some functions params / return types are not documented Without diminishing the astonishing quantity and quality of Robert's work, I'd give a 4th review to the patch set. As for myself, I'll pick all the changes in the modules I maintain and I'll push them (if nobody minds).
- the ChannelAccess enumeration elements are not documented. src/wimax/model/wimax-tlv.h - CommonTypes enumeration elements are not documented - "/// constructor" is redundant (personal opinion), but when it is documented, the params must be documented as well. - some functions params / return types are not documented IMO undocumented elements should not block commit like this; they will still show up as undocumented, and we can come back and fill them in. - the 3 initializers are not explained - "/// constructor" is redundant - Serialize, Deserialize and GetSerializedSize shouldn't be documented (they are virtual, the documentation is inherited from the base class) Uninformative docs are a problem, and should be omitted, since they mislead us into thinking it's done, and makes it much harder to find and fix later.
I have uploaded updates to resolve the review comments. Except for LTE and 2 other files, doxygen warnings for all modules have been resolved. I have created a tarball of the updated files but the size is 8 MB so it wont attach here. If you send me an E mail, I will sent the tarball.
I'm sorry if I'm pedantic, but I'm (slowly) committing patches to the modules I maintain and I'm using Robert's work as a guideline. The main differences are: 1) Triple check the parameters documentation 2) \ingroup and \defgroup use 3) Clearer explanation of the function / member variables. Point 1 could be skipped, just because missing stuff will generate errors. Point 2 is for clarity, and I think it is important. Each module should have a *clear* distinction between classes used in the module, in the examples and in the tests. Moreover, test and example classes should be (also) grouped in a separate meta-group. Little known fact: a class can belong to two or more groups, just use more /ingroup. Point 3 is the most critical / controversial. As much as I hate Doxygen errors, I hate even more to have a function poorly documented (and not generating errors). In this case the user will be unhappy-confused-angry and the badly documented functions will be even harder to spot.
Changes to correct doxygen warnings for the LTE module have been uploaded as http://codereview.appspot.com/315450043
Additional (final?) changes needed to address all review comments for the Antenna module have been uploaded as http://codereview.appspot.com/319190043
I'm going to push a fix for all the changes in the Internet module. (In reply to Robert Ammon from comment #86) > Additional (final?) changes needed to address all review comments for the > Antenna module have been uploaded as http://codereview.appspot.com/319190043
Please note that I'm using this system: 1) At the beginning of one of the tests of each module I a defgroup, e.g.: /** * \ingroup internet * \defgroup internet-test internet module tests */ 2) In each class belonging to the test of each module, I added the proper ingroup(s), e.g.: * \ingroup internet-test * \ingroup tests The "tests" group is defined in the tests module. In this way the classes used in the tests are: 1) Visualized in a single sub-group in the doxygen page, and they don't mess with the module "normal" classes, and 2) Are collected in a single place too.
Additional (final?) changes needed to address all review comments for the AODV module have been uploaded as http://codereview.appspot.com/320060043
Additional (final?) changes needed to address all review comments for the applications module have been uploaded as http://codereview.appspot.com/318350043
Additional (final?) changes needed to address all review comments for the brite module have been uploaded as http://codereview.appspot.com/314360043
Additional (final?) changes needed to address all review comments for the buildings module have been uploaded as http://codereview.appspot.com/317140043
Additional (final?) changes needed to address all review comments for the config-store module have been uploaded as http://codereview.appspot.com/316200043
Additional (final?) changes needed to address all review comments for the core module have been uploaded as http://codereview.appspot.com/318360043
Additional (final?) changes needed to address all review comments for the dsdv module have been uploaded as http://codereview.appspot.com/320070043
Additional (final?) changes needed to address all review comments for the dsr module have been uploaded as http://codereview.appspot.com/319200043
Additional (final?) changes needed to address all review comments for the energy module have been uploaded as http://codereview.appspot.com/318390043
Additional (final?) changes needed to address all review comments for the mesh module have been uploaded as http://codereview.appspot.com/317150043
Additional (final?) changes needed to address all review comments for the mobility module have been uploaded as http://codereview.appspot.com/311600043
Additional (final?) changes needed to address all review comments for the mpi module have been uploaded as http://codereview.appspot.com/318390044
Additional (final?) changes needed to address all review comments for the netanim module have been uploaded as http://codereview.appspot.com/319230043
Additional (final?) changes needed to address all review comments for the network module have been uploaded as http://codereview.appspot.com/318410043
Additional (final?) changes needed to address all review comments for the nix-vector-routing module have been uploaded as http://codereview.appspot.com/319230044
Additional (final?) changes needed to address all review comments for the openflow module have been uploaded as http://codereview.appspot.com/313490043
Additional (final?) changes needed to address all review comments for the propagation module have been uploaded as http://codereview.appspot.com/313500043
Additional (final?) changes needed to address all review comments for the spectrum module have been uploaded as http://codereview.appspot.com/320100043
Additional (final?) changes needed to address all review comments for the stats module have been uploaded as http://codereview.appspot.com/317170043
Additional (final?) changes needed to address all review comments for the test module have been uploaded as http://codereview.appspot.com/316240043
Additional (final?) changes needed to address all review comments for the traffic-control module have been uploaded as http://codereview.appspot.com/315510043
Additional (final?) changes needed to address all review comments for the uan module have been uploaded as http://codereview.appspot.com/319260043
Additional (final?) changes needed to address all review comments for the virtual-net-device module have been uploaded as http://codereview.appspot.com/320110043
Additional (final?) changes needed to address all review comments for the visualizer module have been uploaded as http://codereview.appspot.com/319270043
Additional (final?) changes needed to address all review comments for the wave module have been uploaded as http://codereview.appspot.com/311610044
Additional (final?) changes needed to address all review comments for the utils module have been uploaded as http://codereview.appspot.com/320120043
Additional (final?) changes needed to address all review comments for the examples module have been uploaded as http://codereview.appspot.com/318430043
Additional (final?) changes needed to address all review comments for the wimax module have been uploaded as http://codereview.appspot.com/318460043
Additional (final?) changes needed to address all review comments for the wifi module have been uploaded as http://codereview.appspot.com/313560043. There is one file model/vht-wifi-mac-helper.h that I have not been able to figure out the solution to the doxygen warning. If someone else could take a look at that file and see what the resolution is.
Additional (final?) changes needed to address all review comments for the lte module have been uploaded as http://codereview.appspot.com/311650043
Additional changes needed to correct warning for the latest changes to the internet module have been uploaded as http://codereview.appspot.com/313570043
I have completed all updates required to correct the doxygen warnings. There are two files that have one warning each left, I have not been able to determine the correction(s) required. The affected files are: Count File ----- ---------------------------------- 1 src/visualizer/visualizer/ipython_view.py 1 src/wifi/helper/vht-wifi-mac-helper.h ---------------------------------------- 2 files with warnings I would like to see all of the changes incorporated ASAP. The changes to LTE and WIFI over the last week or two have introduced 100-200 new doxygen warnings that I have corrected. IMHO, the only way to permanently solve the issue of doxygen warnings is to get these corrections merged and then modify the master build options to fail a build if there are doxygen warnings so they are corrected immediately. Not only will this ensure that doxygen warnings to not creep back into the project, it will also improve the doxygen comments since they will be made by the original developer which should be much higher in quality than those made later by maintainers.
(In reply to Robert Ammon from comment #120) > I have completed all updates required to correct the doxygen warnings. > There are two files that have one warning each left, I have not been able to > determine the correction(s) required. The affected files are: > > Count File > ----- ---------------------------------- > 1 src/visualizer/visualizer/ipython_view.py > 1 src/wifi/helper/vht-wifi-mac-helper.h > ---------------------------------------- > 2 files with warnings > > I would like to see all of the changes incorporated ASAP. The changes to > LTE and WIFI over the last week or two have introduced 100-200 new doxygen > warnings that I have corrected. > > IMHO, the only way to permanently solve the issue of doxygen warnings is to > get these corrections merged and then modify the master build options to > fail a build if there are doxygen warnings so they are corrected > immediately. Not only will this ensure that doxygen warnings to not creep > back into the project, it will also improve the doxygen comments since they > will be made by the original developer which should be much higher in > quality than those made later by maintainers. Robert, thank you for this great effort. I agree that we should treat future warnings as a build error and clean them immediately.
Additional changes needed to correct warning for the latest changes to the olsr module have been uploaded as http://codereview.appspot.com/318550043
Status (Feb 17 2017): modules still requiring patches to be committed: antenna aodv brite buildings config-store core dsdv dsr energy lte mesh mpi netanim network nix-vector openflow spectrum utils virtual-net-device visualizer wave wifi wimax
A couple of updates to Tom's comment. 1) Wifi should be complete. There still some doxygen errors in the output but those should be corrected once the changes for Core are merged. 2) One additional item that needs to be included is Examples (directly under the ns-3-dev folder). 3) I have been downloading other changes as they are being made and updating them as needed to correct additional doxygen errors introduced. In this case, I have updated the patch set posted to Reitveld as needed. Modules where the most recent changes need to be merged are Test and Examples.
List of modules that still need merged should also include applications.
The Propagation module should be added to the list of outstanding modules.
(In reply to Robert Ammon from comment #114) > Additional (final?) changes needed to address all review comments for the > utils module have been uploaded as http://codereview.appspot.com/320120043 Additional changes uploaded with corrections for latest external changes.
Latest doxygen warning status Report of Doxygen warnings ---------------------------------------- (All counts are lower bounds.) Warnings by module/directory: Count Directory ----- ---------------------------------- 1530 src/wimax/model 513 src/core/test 507 src/mesh/model 261 src/netanim/model 248 src/visualizer/visualizer 227 src/dsr/model 192 src/wave/model 156 src/core/model 127 src/aodv/model 123 src/visualizer/model 109 src/test/ns3tcp 108 src/config-store/model 106 src/network/test 89 src/wave/examples 85 src/mpi/model 74 src/dsdv/model 65 src/wifi/model 65 src/mobility/test 63 src/propagation/test 63 src/mesh/test 61 src/test/traced 61 src/energy/model 61 src/buildings/model 55 src/wave/test 49 src/antenna/test 46 src/openflow/model 45 src/test/csma-system-test-suite.cc 44 src/spectrum/test 44 src/buildings/test 39 src/wave/helper 39 examples/wireless/multirate.cc 37 src/buildings/helper 31 src/aodv/test 30 examples/wireless/power-adaptation-interference.cc 28 src/brite/helper 26 src/dsdv/examples 26 examples/stats/wifi-example-apps.h 25 src/stats/test 23 src/netanim/examples 23 examples/wireless/power-adaptation-distance.cc 22 src/wimax/test 22 src/nix-vector-routing/model 22 examples/wireless/mixed-network.cc 21 src/stats/examples 19 src/spectrum/model 18 src/virtual-net-device/examples 18 src/antenna/model 17 src/virtual-net-device/model 17 src/test/ns3wifi 17 src/netanim/test 17 src/energy/examples 16 src/energy/helper 15 src/mesh/helper 15 src/mesh/examples 15 examples/routing/manet-routing-compare.cc 14 src/dsr/test 12 examples/tutorial/fifth.cc 10 src/nix-vector-routing/examples 10 src/network/examples 9 src/wimax/helper 9 src/test/ns3tc 9 src/energy/test 8 src/olsr/test 8 src/dsr/helper 8 src/core/examples 8 src/aodv/examples 8 examples/wireless/wifi-clear-channel-cmu.cc 8 examples/wireless/rate-adaptation-distance.cc 7 examples/wireless/wifi-adhoc.cc 6 src/propagation/examples 4 src/network/utils 4 examples/tutorial/sixth.cc 4 examples/tutorial/seventh.cc 3 src/wifi/helper 3 src/openflow/test 3 src/lte/test 3 src/dsdv/test 3 src/config-store/examples 3 src/brite/test 2 src/traffic-control/model 2 src/flow-monitor/model 2 examples/tutorial/fourth.cc 1 src/stats/model 1 src/applications/test 1 examples/wireless/wifi-spectrum-per-interference.cc 47 additional undocumented parameters. ---------------------------------------- 5995 total warnings 85 directories with warnings
Doxygen corrections for latest DHCP modification to module internet-apps has been uploaded to http://codereview.appspot.com/329800043
In trying to finish up the Doxygen work, I'm having some difficulty with suppressing warnings due to macro expansions. This is with the latest Doxygen release (1.8.13). As of changeset 13048:c00059ef50e4, Doxygen warnings report is reporting 64 warnings in the wifi module. These are all due to macro expansions; e.g.: Filtered Warnings ======================================== src/wifi/model/dsss-parameter-set.cc:100: warning: return type of member ns3::MakeDsssParameterSetChecker is not documented src/wifi/model/dsss-parameter-set.h:111: warning: Compound ns3::DsssParameterSetValue is not documented. src/wifi/model/dsss-parameter-set.h:111: warning: Member DsssParameterSetValue(const DsssParameterSet &value) (function) of class ns3::DsssParameterSetValue is not documented. from /// DsssParameterSet ATTRIBUTE_HELPER_CPP (DsssParameterSet); I looked at doxygen.conf and found that these were all listed for expansion under the EXPAND_AS_DEFINED setting. I'm not sure why we need anything documented more than the original macro definition in src/core/model/attribute-helper.h. So I disabled these in doxygen.conf: -EXPAND_AS_DEFINED = ATTRIBUTE_ACCESSOR_DEFINE \ - ATTRIBUTE_CHECKER_DEFINE \ - ATTRIBUTE_CHECKER_IMPLEMENT \ - ATTRIBUTE_CHECKER_IMPLEMENT_WITH_NAME \ - ATTRIBUTE_CONVERTER_DEFINE \ - ATTRIBUTE_HELPER_CPP \ - ATTRIBUTE_HELPER_HEADER \ - ATTRIBUTE_VALUE_DEFINE \ - ATTRIBUTE_VALUE_DEFINE_WITH_NAME \ - ATTRIBUTE_VALUE_IMPLEMENT \ - ATTRIBUTE_VALUE_IMPLEMENT_WITH_NAME +EXPAND_AS_DEFINED = but I still get a number of warnings as a result: Filtered Warnings ======================================== src/wifi/model/dsss-parameter-set.h:111: warning: Member ATTRIBUTE_HELPER_HEADER(DsssParameterSet) (function) of namespace ns3 is not documented. src/wifi/model/edca-parameter-set.h:408: warning: Member ATTRIBUTE_HELPER_HEADER(EdcaParameterSet) (function) of namespace ns3 is not documented. src/wifi/model/erp-information.cc:124: warning: Member ATTRIBUTE_HELPER_CPP(ErpInformation) (function) of namespace ns3 is not documented. I didn't expect those since the SKIP_FUNCTION_MACROS tag is set to YES. Any ideas? If Doxygen can't handle these macros, should we filter these out in doxygen.warnings.report.sh?
The current doxygen.conf is correct. Attribute documentation is created by utils/print-introspected-doxygen.cc:866 This ensures that the symbols created by macro expansion *do* get documented. Prior to this change searching for FooValue would return nothing, which is frustrating for new users. This is documented in ATTRIBUTE_HELPER_HEADER: https://www.nsnam.org/doxygen/group__attributehelper.html#details I grepped for all uses of ATTRIBUTE_VALUE_HEADER and found these new ones missing from the list in print-introspected-doxygen.cc: DsssParameterSet EdcaParameterSet ErpInformation HeCapabilities HtOperation VhtCapabilities VhtOperation
(In reply to Peter Barnes from comment #131) > The current doxygen.conf is correct. Attribute documentation is created by > utils/print-introspected-doxygen.cc:866 Thanks for clarifying; I did not think to look there. I will patch print-introspected-doxygen.cc - Tom
Current status of this issue: Report of Doxygen warnings ---------------------------------------- (All counts are lower bounds.) Warnings by module/directory: Count Directory ----- ---------------------------------- 396 src/core/test 110 src/test/ns3tcp 104 src/lte/model 91 src/mpi/model 88 src/core/model 81 src/internet/test 65 src/buildings/model 63 src/test/traced 63 src/propagation/test 61 src/energy/model 49 src/antenna/test 46 src/openflow/model 46 src/buildings/test 45 src/test/csma-system-test-suite.cc 44 src/spectrum/test 39 src/propagation/model 39 examples/wireless/wifi-multirate.cc 38 src/buildings/helper 33 src/config-store/model 31 src/wifi/test 30 examples/wireless/wifi-power-adaptation-interference.cc 28 src/mobility/helper 28 src/internet/model 28 src/brite/helper 26 src/wifi/model 26 examples/stats/wifi-example-apps.h 25 src/stats/test 23 src/core/examples 23 examples/wireless/wifi-power-adaptation-distance.cc 22 examples/wireless/wifi-mixed-network.cc 21 src/test/ns3wifi 21 src/stats/examples 18 src/virtual-net-device/model 18 src/virtual-net-device/examples 18 src/antenna/model 18 doc/introspected-doxygen.h 17 src/energy/examples 16 src/energy/helper 15 examples/routing/manet-routing-compare.cc 14 src/stats/model 14 src/spectrum/model 12 examples/tutorial/fifth.cc 9 src/uan/model 9 src/test/ns3tc 9 src/energy/test 8 src/traffic-control/model 8 src/mobility/test 8 examples/wireless/wifi-rate-adaptation-distance.cc 8 examples/wireless/wifi-clear-channel-cmu.cc 8 examples/wireless/wifi-adhoc.cc 6 src/propagation/examples 6 src/mobility/model 5 src/wave/model 5 src/fd-net-device/helper 4 examples/tutorial/sixth.cc 4 examples/tutorial/seventh.cc 3 src/visualizer/model 3 src/traffic-control/test 3 src/openflow/test 3 src/fd-net-device/model 3 src/brite/test 2 src/wave/helper 2 src/visualizer/visualizer 2 examples/wireless/wifi-80211e-txop.cc 2 examples/tutorial/fourth.cc 1 utils/print-introspected-doxygen.cc 1 src/wifi/helper 1 src/traffic-control/helper 1 src/tap-bridge/model 1 src/spectrum/helper 1 src/mesh/model 1 examples/wireless/wifi-txop-aggregation.cc 1 examples/wireless/wifi-spectrum-per-interference.cc 24 additional undocumented parameters. ---------------------------------------- 2145 total warnings 73 directories with warnings
Excluding examples and tests: Report of Doxygen warnings ---------------------------------------- (All counts are lower bounds.) Warnings by module/directory: Count Directory ----- ---------------------------------- 104 src/lte/model 91 src/mpi/model 88 src/core/model 65 src/buildings/model 61 src/energy/model 46 src/openflow/model 39 src/propagation/model 38 src/buildings/helper 33 src/config-store/model 28 src/mobility/helper 28 src/internet/model 28 src/brite/helper 26 src/wifi/model 18 src/virtual-net-device/model 18 src/antenna/model 18 doc/introspected-doxygen.h 16 src/energy/helper 14 src/stats/model 14 src/spectrum/model 9 src/uan/model 8 src/traffic-control/model 6 src/mobility/model 5 src/wave/model 5 src/fd-net-device/helper 3 src/visualizer/model 3 src/fd-net-device/model 2 src/wave/helper 2 src/visualizer/visualizer 1 utils/print-introspected-doxygen.cc 1 src/wifi/helper 1 src/traffic-control/helper 1 src/tap-bridge/model 1 src/spectrum/helper 1 src/mesh/model 24 additional undocumented parameters. ---------------------------------------- 846 total warnings 34 directories with warnings
Is there's still something to be done here?
(In reply to Parth Pandya from comment #135) > Is there's still something to be done here? Yes. The current counts are: $ ./doc/doxygen.warnings.report.sh -e -t Count Directory ----- ---------------------------------- 104 src/lte/model 85 src/mpi/model 61 src/energy/model 53 src/buildings/model 46 src/openflow/model 38 src/buildings/helper 28 src/internet/model 28 src/brite/helper 22 src/wifi/model 22 src/core/model 18 src/antenna/model 17 src/virtual-net-device/model 16 src/energy/helper 13 src/traffic-control/model 10 src/network/utils 7 src/uan/model 6 src/spectrum/model 2 src/mesh/model 2 src/lr-wpan/model 1 src/wifi/helper 1 src/traffic-control/helper 1 src/stats/model 1 src/config-store/model 1 doc/introspected-doxygen.h 26 additional undocumented parameters. ---------------------------------------- The warnings will be left in doc/doxygen-warnings.log. Robert Ammon has created patches for some modules (see links in previous comments). We need to check that these patches still apply and to audit the changes.
I suggest that if volunteers want to work on these, that they take the following steps: 1) reproduce the doxygen warnings log for themselves (check the latest status) 2) pick a module of interest. Make sure that there is no pending merge request for the module of interest. 3) look above to see whether Robert has already created a patch for that module. If so, use it as a starting point. 4) Create a patch to ns-3-dev to clear the remaining errors of that module. The changes must be valid, useful documentation; not just small edits for the sake of clearing the warning. 5) submit a merge request, and a maintainer will review and either apply the patch, or ask for more work. Repeat for next module, as necessary.
Proposal to close
(In reply to Tommaso Pecorella from comment #138) > Proposal to close Blast from the past! Please add a reference to the GitLab issue or MR which closes this.
plus, we should schedule the celebration. At one point, this issue looked like a daunting one to close out.
Closing this issue as Doxygen doesn't raise any serious warning anymore (as of ns-3.41). Moreover, the new CI searches for Doxygen warnings and we're fairly sure we're going to fix them promptly when they're spotted. Hence... closing and celebrating.
(In reply to Tommaso Pecorella from comment #141) > Hence... closing and celebrating. WOO HOO!