Skip to content

Remove template walls #7303

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

Closed
wants to merge 1 commit into from
Closed

Conversation

geky
Copy link
Contributor

@geky geky commented Jun 22, 2018

Description

By selectively removing doxygen comments, we can present only the useful function documentation to users. Note that in C++11 these would just be vararg functions.

Still included are a single overload for functions + args, member function + args, and function + state + args.

In:

  • EventQueue.h
    • call
    • call_in
    • call_every
    • event
  • Event
    • constructor
  • Callback
    • constructor
    • attach
    • callback

cc @AnotherButler, @pan-, @kegilbert

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@pan-
Copy link
Member

pan- commented Jun 22, 2018

What's the impact on doxygen rendering ?

@geky
Copy link
Contributor Author

geky commented Jun 22, 2018

@pan- I'm struggling on how to show it before the CI runs, but we should now only see one set of function overloads instead of every set of overloads for each parameter list:

From:
image

To:
image

@geky geky force-pushed the remove-template-walls branch from 892576b to 01c6c3e Compare June 22, 2018 18:09
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

A better option would be to have a single documentation with varargs for doxygen:

#if defined(DOXYGEN_ONLY)

// single signature and documentation

#else 

// regular code

#endif

It would clean the output and avoid error from the CI jobs that test there is no warnings in the doc.

@geky geky force-pushed the remove-template-walls branch from 01c6c3e to ba9fdf5 Compare June 22, 2018 18:17
@geky
Copy link
Contributor Author

geky commented Jun 22, 2018

Oh I see, we could do that too, although it would clutter the header file.

I'm not sure it's needed, the CI errors were about a misnamed parameter.

@geky geky force-pushed the remove-template-walls branch from ba9fdf5 to bf32121 Compare June 22, 2018 18:21
By selectively removing doxygen comments, we can present only the
useful function documentation to users. Note that in C++11 these would
just be vararg functions.

In:
- EventQueue.h
  - call
  - call_in
  - call_every
  - event
- Event
  - constructor
- Callback
  - constructor
  - attach
  - callback
@geky geky force-pushed the remove-template-walls branch from bf32121 to 66b0ee2 Compare June 22, 2018 18:32
@pan-
Copy link
Member

pan- commented Jun 22, 2018

I got no strong opinion on this; the advantage is the possibility to use variadic templates syntaxe for the documentation. It may make the intent clearer rather than having the function with the most overloads.

@geky
Copy link
Contributor Author

geky commented Jun 22, 2018

Oh! I understand now. That makes sense, let's see what that looks like.

@cmonr
Copy link
Contributor

cmonr commented Jul 3, 2018

@geky Making a note here that this is still open.

@cmonr
Copy link
Contributor

cmonr commented Jul 9, 2018

Closing this since no update has occurred in two weeks.

Feel free to reopen once an update is available.

@cmonr cmonr closed this Jul 9, 2018
@AnotherButler
Copy link
Contributor

@geky Please update this. This was requested externally.

@geky
Copy link
Contributor Author

geky commented Jul 9, 2018

Currently working on it on the branch outside of this pr, will reopen when ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants