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 invoke_fused and friends NFC. #2611

Merged
merged 1 commit into from May 30, 2017

Conversation

Projects
None yet
3 participants
@Naios
Copy link
Contributor

commented May 6, 2017

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

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

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));
}

This comment has been minimized.

Copy link
@K-ballo

K-ballo May 6, 2017

Member

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.

This comment has been minimized.

Copy link
@Naios

Naios May 6, 2017

Author Contributor

Shall I leave the simplification out of the commit then?

This comment has been minimized.

Copy link
@K-ballo

K-ballo May 6, 2017

Member

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.

This comment has been minimized.

Copy link
@Naios

Naios May 6, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@hkaiser

hkaiser May 6, 2017

Member

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.

This comment has been minimized.

Copy link
@Naios

Naios May 8, 2017

Author Contributor

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

@hkaiser

This comment has been minimized.

Copy link
Member

commented May 6, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

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

@Naios

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2017

@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.

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

This comment has been minimized.

Copy link
@K-ballo

K-ballo May 21, 2017

Member

where does this throws clause come from?

This comment has been minimized.

Copy link
@Naios

Naios May 21, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@K-ballo

K-ballo May 22, 2017

Member

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

This comment has been minimized.

Copy link
@Naios

Naios May 30, 2017

Author Contributor

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).

screenshot_4

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

This comment has been minimized.

Copy link
@K-ballo

K-ballo May 21, 2017

Member

What's the meaning of "functional type"?

This comment has been minimized.

Copy link
@Naios

Naios May 21, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@K-ballo

K-ballo May 22, 2017

Member

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.

This comment has been minimized.

Copy link
@Naios

Naios May 30, 2017

Author Contributor

Thanks for the suggestion, I changed this accordingly.

@hkaiser

This comment has been minimized.

Copy link
Member

commented May 29, 2017

@Naios is there anything needed to move this forward?

@Naios

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

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

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

@Naios

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2017

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

@hkaiser
Copy link
Member

left a comment

Thanks, LGTM!

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.