Fix Doxygen warnings unrelated to documentation #3701
Conversation
|
Actually I seem to have 1.8.17 whereas CI uses 1.8.13 which means I'm introducing some tags which are still unsupported and there are some different warnings which I don't see as well. Also, I didn't try to build the LaTeX, Sphinx, ... docs, only the HTML stuff... Maybe there's more points to the meta-issue than what I realized.
|
|
First of all, the merge with master introduced 19(!) new warnings (mostly) related to With the downgrade there's a lot of missing warnings and one new (of course after removing the extra tags): I'll add an empty ballot box for the PCL_DEPRECATED ones in the OP and fix what I can of the rest down here. EDIT: apart from the clang-format-reintroduced-warning "Illegal command @param as part of a \a command", the rest of the new ones are all due to PCL_DEPRECATED. It could be a matter of setting some value to it in the doxyfile (?) |
|
Thank you... honestly... thank you :D What happened in this commit 87d2d80 ? At first sight it looks like a bunch of indentation changes which should not be required to get the warnings sorted. |
|
The indentation was later corrected by clang-format, the real change to remove some of the warnings was with the namespace declarations. As I wrote here it's also against your rules though:
|
|
Seriously, thanks. This is something that didn't even cross our minds (not mine at least). I don't know much about why the second part of rule 2.1 was chosen. |
|
No problem, at least I hope to have the time to finish it. Regarding the I can get most of the deprecation warnings in the docs as well (bonus!), except (at least I noticed these) the typedefs in pcl_macros.h. For those, if I were to write (so a reverse order) I would get the docs, but also a compilation error only in these lines. What do you think? Also en passant, there are 76 instances where Related doxygen/doxygen#5940 |
As you have probably realized already, on CI we generate documentation in a container. Here is the Dockerfile. So the two options are:
In case you prefer the second option, we may switch to 20.04 alpha image, however even that has 1.8.16, which lags behind. |
|
It's fine, I downgraded my local build. Thanks for the docker anyways, so I can experiment with sphinx! |
|
Note to maintainers: This PR shows a need to update the namespace related style guide. |
kunaltyagi
added
changelog: fix
aPonza
force-pushed
the
fix_doc_warnings
branch
from
ff55d47
to
3d12afe
Compare
|
Which one should we give priority to? I was inclined towards #3760. It looked more atomic. |
|
You can start with that one: even though to me it depends on this one it's basically unrelated BTW: |
There was a problem hiding this comment.
The changes mainly belong in 4 categories:
- Doxygen file config
- Documentation change
pcl::module::name->namespace pcl::module { name }- white space modifications
Does it make sense to create separate PRs for documentation and doxygen config? That'll allow the last 2 to be merged (in one PR) along with a change to the style guide.
FYI: Checked the last 3 and I'm onboard with them. (The whitespace changes don't matter since clang-format will reformat them anyways). Haven't checked the doxygen config though
Sidenote: Does it make sense to have a diff output on the doc CI job to see what changes?
You want a cherry-pick for the remaining namespace changes and the rest left here? Sure can do, I can actually push everything on #3760 directly, there should be no conflict. Documentation and config I think can stay here as the two separate commits of this PR, but I'll wait for your opinions.
You mean of the warnings? Sounds weird and hopefully soon unnecessary? |
No, no. Not the diff of the warnings, but diff of the doc. When we modify the documentation, we don't get any feedback till it is merged. But we can conditionally, diff wrt the master branch documentation and show it in the CI output. That'll allow things like 'Did the autogen work?' or 'Does it look more like how I wanted' be a bit readable using typical html and css diffs.
Yeah, that'd be really nice.
That also works 😄 , when I made the comments, I didn't notice the other PR. I'll take a look at the config file sometime later today |
That sounds nice to have! I'll go ahead and push the rebase then. |
aPonza
force-pushed
the
fix_doc_warnings
branch
2 times, most recently
from
33c9f75
to
de08662
Compare
There was a problem hiding this comment.
LGTM. Thank you. Once @kunaltyagi's raised points are addressed, feel free to merge from my side.
kunaltyagi
added
needs: more work
… generation of deprecation list, increasing dot graph max nodes and fixing one instance of 'no matching file member found for ' file_io.hpp
aPonza
changed the title
aPonza
force-pushed
the
fix_doc_warnings
branch
from
de08662
to
1fb689d
Compare
kunaltyagi
changed the title
taketwo
changed the title
Building the documentation currently yields 146 warnings:
Build output
Removing them would eventually allow to use the equivalent to -Werror in the Documentation CI job. Some notes:
This PR currently leaves
7172 warnings, of which:3019 for the no (uniquely) matching warnings (1 fromPCL_DEPRECATED);1516 for parameter not found (see breakdown down here, of which 4 fromPCL_DEPRECATED);PCL_DEPRECATED.Some notes on the missing warnings:
/** \cond NO_WARN_RECURSIVE */-/** \endcond */pair for all four traits (as per this SO answer) but it'd need a new header: just adding the pair leads to "warning: Conditional section with label 'NO_WARN_RECURSIVE' does not have a corresponding \endcond command within this file.". The solutions seems to be movingpcl/common/include/pcl/point_traits.h
Lines 130 to 187 in dac8961
pcl/common/include/pcl/register_point_struct.h
Lines 316 to 345 in dac8961
pcl/common/include/pcl/register_point_struct.h
Lines 351 to 356 in dac8961
point_traits.hto remove warning during doc generation by doxygen #3712;pcl/registration/include/pcl/registration/gicp6d.h
Lines 77 to 85 in dac8961
PCL_DEPRECATED. These might be solvable with some\cond-\endcondpair which I still haven't figured out. The last 2 in the comment are actually hidden by the fact we're using 1.8.13.Also, exhuming #642 for reference.