Add documentation to invoke_fused and friends NFC. #2611

Naios · 2017-05-06T11:17:26Z

Clearify the meaning of invoke_fused through
using invoke_fused_r instead of the internal implementation.

K-ballo · 2017-05-06T12:14:22Z

hpx/util/invoke_fused.hpp

-            typename detail::fused_index_pack<Tuple>::type());
+        typedef typename detail::fused_result_of<F&&(Tuple&&)>::type R;
+
+        return invoke_fused_r<R>(std::forward<F>(f), std::forward<Tuple>(t));
    }


The original implementation attempted to reduce the number of entries on the stack (noise in stack traces, steps while debugging, etc), resulting in such de-simplifications.

Shall I leave the simplification out of the commit then?

Either way is fine with me, I don't feel strongly about it. I merely wanted to expose the underlying rationale for that implementation.

On that note, if the decision is to simplify then perhaps similar changes should be applied to invoke as well, which followed the same rationale.

Well, thanks for pointing that out.

I didn't intend to apply similar changes, just spotted that invoke_fused isn't self explaining
thus I wanted to add documentation to it.
invoke (and its underlying implementation) seems good for me since it's self-explanatory.

I think, the same changes were applied to invoke as well. Also, this change was also triggered by deficiencies in certain C++ compilers (notably NVCC) which was not able to compile things without those changes.

I removed the simplification changes, since those aren't worth a broken build on a non popular toolchain.

hkaiser · 2017-05-06T13:23:37Z

Thanks for working on this. This is much appreciated!

In order to get this into the docs you will have to add the file to the list of files parsed by doxygen (see here: https://github.com/STEllAR-GROUP/hpx/blob/master/docs/CMakeLists.txt#L46-L174). Also, you may want to consider creating a documentation index entry (http://stellar-group.github.io/hpx/docs/html/index/s05.html) for invoke and invoke_fused here: https://github.com/STEllAR-GROUP/hpx/blob/master/docs/hpx.idx.

Also, if we start adding doxygen comments for those functions, it might be nice to add descriptions for what the function is doing and for the function argument values as well (see doxygen \param directive), see here for an example: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/lcos/when_all.hpp#L73-L97. A note about exception handling might be good as well (i.e. does not throw any exceptions except if the invoked function or the copy/move constructors of the arguments do...)

Naios · 2017-05-06T13:30:19Z

Thanks @hkaiser for your explanation, I will improve this commit in order to add the description of the function into the documentation (while adding the missing entry for the exception behaviour).

Naios · 2017-05-08T21:04:08Z

The \copydoc on invoke_fused_r and invoke_r doesn't work properly as reported in #2615.

Naios · 2017-05-21T15:32:47Z

@hkaiser The PR was updated recently to also include invoke and invoke_r.
Also the index entries were added.

I just rebased the PR so it should be ready for merging as soon as the CI reports green.

K-ballo · 2017-05-21T22:58:13Z

hpx/util/invoke.hpp

+    ///          the given argument types.
+    ///
+    /// \throws std::exception thrown from a function
+    ///         invocation of f with the argument types vs.


where does this throws clause come from?

Doxygen provides it as a part of its API: \throws <exception-object> { exception description }.
The specification of a possible implicit thrown exception was requested above.

Sorry, I meant what part of the implementation does this clause correspond to? In particular, what restricts exceptions to be of type std::exception?

I clearified this and said std::exception like type instead. The issue is that doxygen expects a particular typename after \throws, otherwise it will report errors during the documentation generation (as shown below).

K-ballo · 2017-05-21T22:58:40Z

hpx/util/invoke.hpp

+    /// Invokes the given functional type f with the content of
+    /// the argument pack vs
+    ///
+    /// \param f A functional type providing an `operator()`


What's the meaning of "functional type"?

I meant an object invokeable through an operator() like function pointers or objects overloading operator().

I believe the term you are looking for is callable object (f is not a type), which is comprised by function objects and member pointers.

Thanks for the suggestion, I changed this accordingly.

hkaiser · 2017-05-29T16:31:53Z

@Naios is there anything needed to move this forward?

Naios · 2017-05-29T20:20:24Z

@hkaiser I will finish this PR tomorrow due to my first GSoC working day, through applying the suggested improvements by K-ballo.

Naios · 2017-05-30T11:27:03Z

@hkaiser I added the suggested improvements and thus the PR should be ready for merge.

hkaiser

Thanks, LGTM!

Naios force-pushed the fused branch from ee1c1f5 to 26f545d Compare May 6, 2017 11:21

K-ballo reviewed May 6, 2017

View reviewed changes

hkaiser added this to the 1.1.0 milestone May 6, 2017

hkaiser added category: documentation type: enhancement labels May 6, 2017

Naios force-pushed the fused branch from 26f545d to e58a2f0 Compare May 8, 2017 20:33

Naios mentioned this pull request May 8, 2017

Documentation: Linking functions is broken #2615

Closed

Naios mentioned this pull request May 8, 2017

Documentation: Extract all entities in order to autolink functions co… #2616

Merged

Naios force-pushed the fused branch from e58a2f0 to 272dba4 Compare May 21, 2017 15:31

K-ballo reviewed May 21, 2017

View reviewed changes

Add documentation to invoke_fused and invoke

e890af4

Naios force-pushed the fused branch from 272dba4 to e890af4 Compare May 30, 2017 11:25

hkaiser approved these changes May 30, 2017

View reviewed changes

hkaiser merged commit 6d8baf3 into STEllAR-GROUP:master May 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation to invoke_fused and friends NFC. #2611

Add documentation to invoke_fused and friends NFC. #2611

Naios commented May 6, 2017

K-ballo May 6, 2017

Naios May 6, 2017

K-ballo May 6, 2017

Naios May 6, 2017

hkaiser May 6, 2017

Naios May 8, 2017

hkaiser commented May 6, 2017

Naios commented May 6, 2017

Naios commented May 8, 2017

Naios commented May 21, 2017

K-ballo May 21, 2017

Naios May 21, 2017 •

edited

K-ballo May 22, 2017

Naios May 30, 2017 •

edited

K-ballo May 21, 2017

Naios May 21, 2017

K-ballo May 22, 2017

Naios May 30, 2017

hkaiser commented May 29, 2017

Naios commented May 29, 2017

Naios commented May 30, 2017

hkaiser left a comment

Add documentation to invoke_fused and friends NFC. #2611

Add documentation to invoke_fused and friends NFC. #2611

Conversation

Naios commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser commented May 6, 2017

Naios commented May 6, 2017

Naios commented May 8, 2017

Naios commented May 21, 2017

Choose a reason for hiding this comment

Naios May 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios May 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser commented May 29, 2017

Naios commented May 29, 2017

Naios commented May 30, 2017

hkaiser left a comment

Choose a reason for hiding this comment

Naios May 21, 2017 •

edited

Naios May 30, 2017 •

edited