-
Notifications
You must be signed in to change notification settings - Fork 8
Doc #44
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
Doc #44
Conversation
|
Thanks for the PR @jrouquie, could you maybe say a few words about what changes you've made and why please? |
|
Hi James. @jrouquie is a potential "real world" user of HPCombi. To learn how to use it, he is reading the doc and obviously find a lot of place to improve it. In particular, for different implementation of the same function, I copied with |
|
In README.md, line 8, "compiler" should be "compilers". High Performance Combinatorics in C++ using vector instructions v1.0.1
HPCombi is a C++17 header-only library using the SSE and AVX instruction sets,
-and some equivalents, for very fast manipulation of small combinatorial objects such
-as transformations, permutations, and boolean matrices. The goal
-of this project is to implement various new algorithms and benchmark them on
-various compiler and architectures.
+and some equivalents, for very fast manipulation of small combinatorial objects
+such as transformations, permutations, and boolean matrices. HPCombi implements
+new algorithms and benchmarks them on various compilers and architectures.
``` |
Avoid copy-pasting doc: it's a waste of time for the reader. Just point to relevant docs but have them
Let the reader know they have already read some text (and don't need to read this doc), just point to it if they need to read it again.
7bc7c7a to
2de92a8
Compare
james-d-mitchell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @jrouquie, this is a big improvement to the documentation. Except for the minor comments in the review, can you also please run clang-format on every *.*pp file that you've touched in this PR? Then we can merge this.
|
|
||
| #endif // HPCOMBI_HPCOMBI_HPP_ | ||
|
|
||
| /*! \mainpage HPCombi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that it might be better to just include README.md file as we were previously here, otherwise there's a good chance that the contents of this section and the README.md will become out of sync of over time. Was there a reason for having both? Might it not be better to just update README.md to include the extra material that's included here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contents of this section and the README.md contain distinct information, without repetition. There is no need to check for synchronization. No risk of out of sync.
Indeed, that was precisely the point of this modification : do not copy paste, do not let the reader notice (after having wasted some time) that they have already read this file. Avoid circular link where https://libsemigroups.github.io/HPCombi/ advises to go read "The Doxygen auto generated API" and actually points to itself.
Alternatively one could move the contents of "/*! \mainpage HPCombi" (in hpcombi.hpp) into the readm, and in the main page of the Doxygen doc just point to the readme (instead of duplicating it). I prefered to separate the authors/contributors/thanks list and the technical information, but feel free to implement this alternative solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes perfect sense.
james-d-mitchell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Ok to merge @hivert ?
Looks good to me too ! Please merge it if you have the time. Otherwise I'll do it next week. |
This is a documentation only PR.
Populate the pages
Add introductory text on main Doxygen page.
Remove some repetitions.
Plus smaller stuff.