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

Can documentation be clearer, please? #831

Closed
donaastor opened this issue May 1, 2023 · 9 comments
Closed

Can documentation be clearer, please? #831

donaastor opened this issue May 1, 2023 · 9 comments

Comments

@donaastor
Copy link

This technically barely falls under "issues", but I am having an issue trying to figure out who the actual designer of this algorithm is and trying to find the specifications of it in case I want to implement the algorithm myself. Also, the list of functions and how they are used is hard to find too (I didn't find it). So, please, will someone be kind to make it all more accessible?

@t-mat
Copy link
Contributor

t-mat commented May 1, 2023

who the actual designer of this algorithm

See https://github.com/Cyan4973/xxHash/blob/dev/LICENSE#L1C1-L3

the specifications of it

See https://github.com/Cyan4973/xxHash/tree/dev/doc

the list of functions and how they are used

See https://github.com/Cyan4973/xxHash/blob/dev/Doxyfile

For example,

git clone https://github.com/Cyan4973/xxHash.git
cd xxHash
doxygen
open doxygen/html/index.html

Actually, we should maintain it though.

@t-mat
Copy link
Contributor

t-mat commented May 1, 2023

@Cyan4973, to maintain the document, should we include doxygen as a part of our CI (and treat its warning as error) ?

So far, I've got the following warnings from doxygen command.

$ doxygen
xxhash.h:1320: warning: unable to resolve reference to 'XXH3_128bits_reset_withSecret()' for \ref command
xxhash.h:1543: warning: End of list marker found without any preceding list items
xxhash.h:3231: warning: unable to resolve reference to 'XXH_X86DISPATCH' for \ref command
xxhash.h:1140: warning: unable to resolve reference to 'XXH_STATIC_LINKING_ONLY' for \ref command
xxhash.h:1141: warning: unable to resolve reference to 'XXH_IMPLEMENTATION' for \ref command
xxhash.h:1228: warning: unable to resolve reference to 'XXH_STATIC_LINKING_ONLY' for \ref command
xxhash.h:1229: warning: unable to resolve reference to 'XXH_IMPLEMENTATION' for \ref command
xxhash.h:1237: warning: unable to resolve reference to 'XXH3_createState()' for \ref command
xxhash.h:1237: warning: unable to resolve reference to 'XXH3_freeState()' for \ref command
xxhash.h:1164: warning: unable to resolve reference to 'XXH_STATIC_LINKING_ONLY' for \ref command
xxhash.h:1165: warning: unable to resolve reference to 'XXH_IMPLEMENTATION' for \ref command

@donaastor
Copy link
Author

@t-mat I appreciate your response, thank you. This isn't the first time I couldn't navigate myself on GitHub over something so trivial (I don't know how I fail so miserably), but just in case I am not the only one who is defective, do you think it is a good idea to put these references (at least for specification) on the main page README.md?

@Cyan4973
Copy link
Owner

Cyan4973 commented May 1, 2023

@Cyan4973, to maintain the document, should we include doxygen as a part of our CI (and treat its warning as error) ?

Yes, this is a worthwhile goal

@Cyan4973
Copy link
Owner

Cyan4973 commented Jul 5, 2023

@Cyan4973, to maintain the document, should we include doxygen as a part of our CI (and treat its warning as error) ?

So far, I've got the following warnings from doxygen command.

$ doxygen
xxhash.h:1320: warning: unable to resolve reference to 'XXH3_128bits_reset_withSecret()' for \ref command
xxhash.h:1543: warning: End of list marker found without any preceding list items
xxhash.h:3231: warning: unable to resolve reference to 'XXH_X86DISPATCH' for \ref command
xxhash.h:1140: warning: unable to resolve reference to 'XXH_STATIC_LINKING_ONLY' for \ref command
xxhash.h:1141: warning: unable to resolve reference to 'XXH_IMPLEMENTATION' for \ref command
xxhash.h:1228: warning: unable to resolve reference to 'XXH_STATIC_LINKING_ONLY' for \ref command
xxhash.h:1229: warning: unable to resolve reference to 'XXH_IMPLEMENTATION' for \ref command
xxhash.h:1237: warning: unable to resolve reference to 'XXH3_createState()' for \ref command
xxhash.h:1237: warning: unable to resolve reference to 'XXH3_freeState()' for \ref command
xxhash.h:1164: warning: unable to resolve reference to 'XXH_STATIC_LINKING_ONLY' for \ref command
xxhash.h:1165: warning: unable to resolve reference to 'XXH_IMPLEMENTATION' for \ref command

I could fix the first 2 minor doxygen issue,
but I'm struggling to find a way to fix references to MACROS.
So far, no luck.
It prevents adding a doxygen test to CI, since it would necessarily fail.

@t-mat
Copy link
Contributor

t-mat commented Jul 5, 2023

So far, no luck.
It prevents adding a doxygen test to CI, since it would necessarily fail.

I'll find a way to resolve this issue.

@Cyan4973
Copy link
Owner

@t-mat improvements to the doxygen output should prove helpful for documentation generation.

I presume one possible follow up could be to generate a version after the release, and reference it from the xxhash homepage ?

@t-mat
Copy link
Contributor

t-mat commented Jul 15, 2023

I presume one possible follow up could be to generate a version after the release, and reference it from the xxhash homepage ?

Yes, it's nice to have Google-able document!

I'd also like to recommend to include version in the URL of the document. For example, CMake's document URL contains version : https://cmake.org/cmake/help/v2.8.12/cmake.html
This style improves persistence of the document and encourages users to write/exchange URL directly without worrying about future 404s.

@Cyan4973
Copy link
Owner

Doxygen documentation has been added to homepage :
https://xxhash.com/doc/v0.8.2/index.html

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

No branches or pull requests

3 participants