Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation to hpx::util::unwrapped and hpx::util::unwrapped2. #2660

Merged
merged 1 commit into from Jun 1, 2017

Conversation

Naios
Copy link
Contributor

@Naios Naios commented May 30, 2017

  • Also add some internal comments for improved guidance

The documentation was kept very general so it can be easily extended with the capabilities that are introduced as part of my GSoC project.

}

///////////////////////////////////////////////////////////////////////////
} // end namespace detail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the namespace detail should be wrapped in a pair of /// \cond NOINTERNAL//// \endcond, please see the parallel algorithms for example. This ahides all the implementation detail from doxygen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it seems doxygen hides namespaces named as detail by default. Shall I still add the comments then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I was not aware of that. May I ask to add it anyways for consistency?

Copy link
Contributor Author

@Naios Naios May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually when I think about this, doxygen shouldn't hide the namespace when not specified, because my research about it didn't reveal any default hidden names.

Maybe we could also officially ignore symbols like namespaces named as detail per default:

diff --git a/cmake/templates/autodoc.doxy.in b/cmake/templates/autodoc.doxy.in
index 86861a6..1f5a011 100644
--- a/cmake/templates/autodoc.doxy.in
+++ b/cmake/templates/autodoc.doxy.in
@@ -13,4 +13,5 @@ XML_OUTPUT = @doxygen_output_file@
 OUTPUT_DIRECTORY = @doxygen_output_dir@
 GENERATE_LATEX = NO
 INPUT = @doxygen_inputs@
+EXCLUDE_SYMBOLS = detail

I could open a PR for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this does the trick, by all means, please go ahead.

Copy link
Contributor Author

@Naios Naios May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened the PR (#2665)

@@ -522,6 +580,7 @@ namespace hpx { namespace util
return unwrapped(unwrapped(std::forward<Future>(f)));
}

/// \copydoc unwrapped2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably will not be sufficient to make doxygen generate the docs for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and doxygen generates the documentation for the function correctly:

screenshot_5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh cool (and how surprising ;-) )

* Also add some internal comments for improved guidance
@Naios
Copy link
Contributor Author

Naios commented May 31, 2017

@hkaiser I applied the requested changes to this PR, so it should be ready for merge.

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, now. Thanks!

@hkaiser hkaiser merged commit a12045d into STEllAR-GROUP:master Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants