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 doxygen #42

Merged
merged 60 commits into from
Feb 17, 2024
Merged

Add doxygen #42

merged 60 commits into from
Feb 17, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

In this PR, I have added a template for documentation with doxygen + sphinx. Workflow may be added based on this PR.

@yasahi-hpc yasahi-hpc marked this pull request as draft February 5, 2024 07:40
@yasahi-hpc yasahi-hpc self-assigned this Feb 5, 2024
@yasahi-hpc yasahi-hpc added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 5, 2024
@yasahi-hpc yasahi-hpc changed the title Add doxygen [WIP] Add doxygen Feb 5, 2024
@jbigot jbigot requested a review from pzehner February 5, 2024 10:17
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Great documentation effort! This looks great to me.

file(MAKE_DIRECTORY ${DOXYGEN_OUTPUT_DIR})

# Only regenerate Doxygen when the Doxyfile or public headers change
add_custom_command(OUTPUT ${DOXYGEN_INDEX_FILE}
Copy link
Member

Choose a reason for hiding this comment

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

why not use doxygen_add_docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not notice this command. Seems simpler.

docs/Makefile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why is a Makefile needed in addition to cmake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is for sphinx, and in the end, we do not rely on CMake in the rtd workflow.

docs/conf.py Outdated
# sys.path.insert(0, os.path.abspath('.'))
import subprocess, os

def configureDoxyfile(input_dir, output_dir, doxyfile_in, doxyfile_out):
Copy link
Member

Choose a reason for hiding this comment

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

this same functionality is present in CMake, why is there a duplication?

doxyfile_in = cwd + '/Doxyfile.in'
doxyfile_out = cwd + '/Doxyfile'
configureDoxyfile(input_dir, output_dir, doxyfile_in, doxyfile_out)
subprocess.call('pwd; ls -lat; doxygen Doxyfile; ls -lat doxygen/xml', shell=True)
Copy link
Member

Choose a reason for hiding this comment

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

here again I don't understand, does CMake handle Doxygen call or python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the rtd workflow, only the python works. CMake is not used.

extents_type m_in_extents, m_out_extents;

// Only used when transpose needed
//! Internal buffers used for transpose
Copy link
Member

Choose a reason for hiding this comment

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

without @{ @}, you only comment the first next variable m_in_T, not the following one m_out_T

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, we need the comment like this?

//@{ //! Internal buffers used for transpose @}//

Copy link
Member

Choose a reason for hiding this comment

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

not exactly, the @{ and @} should surround the variables, see https://www.doxygen.nl/manual/grouping.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So,

///@{ 
  //! extents of input/output views
  extents_type m_in_extents, m_out_extents;
///@}

Comment on lines +101 to 103
//! Internal buffers used for transpose
nonConstInViewType m_in_T;
nonConstOutViewType m_out_T;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//! Internal buffers used for transpose
nonConstInViewType m_in_T;
nonConstOutViewType m_out_T;
//! @{
//! Internal buffers used for transpose
nonConstInViewType m_in_T;
nonConstOutViewType m_out_T;
//! @}

@yasahi-hpc yasahi-hpc changed the title [WIP] Add doxygen Add doxygen Feb 17, 2024
@yasahi-hpc yasahi-hpc marked this pull request as ready for review February 17, 2024 07:16
@yasahi-hpc yasahi-hpc merged commit 66c3000 into main Feb 17, 2024
13 of 17 checks passed
@yasahi-hpc yasahi-hpc mentioned this pull request Feb 17, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants