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

DOC: Add support for documenting C/C++ via Doxygen & Breathe #18884

Merged
merged 6 commits into from Sep 5, 2021

Conversation

seiko2plus
Copy link
Member

@seiko2plus seiko2plus commented May 2, 2021

Add support for documenting C/C++ via Doxygen & Breathe

Due to the urgent need to generate C/C++ API reference documentation from comment blocks, especially with
the most recent expansion in the use of SIMD, and also due to the future reliance on C++.

TODO:

  • improves doc examples
  • add release note

@seiko2plus seiko2plus force-pushed the breathe_doxygen branch 2 times, most recently from 7cab7f7 to 515cd8d Compare May 2, 2021 05:00
@seiko2plus seiko2plus added 04 - Documentation 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes labels May 2, 2021
@seiko2plus seiko2plus force-pushed the breathe_doxygen branch 3 times, most recently from 14d7a84 to 79b721a Compare May 3, 2021 13:09
@seiko2plus seiko2plus force-pushed the breathe_doxygen branch 2 times, most recently from 6722d3d to 64382e2 Compare May 28, 2021 20:41
@seiko2plus seiko2plus marked this pull request as ready for review July 21, 2021 20:57
@seiko2plus seiko2plus changed the title WIP, DOC: Add support for documenting C/C++ via Doxygen & Breathe DOC: Add support for documenting C/C++ via Doxygen & Breathe Jul 21, 2021
@seiko2plus seiko2plus force-pushed the breathe_doxygen branch 2 times, most recently from 9123690 to d4f7330 Compare July 25, 2021 03:11
@charris
Copy link
Member

charris commented Aug 18, 2021

@seiko2plus Where does the new documentation go and how is it run. Perhaps that information could be included in the release note.

@seiko2plus
Copy link
Member Author

Where does the new documentation go and how is it run

This patch doesn't generate any documentation from the sources, just initialize Doxygen and Breathe. see
https://21153-908607-gh.circle-artifacts.com/0/doc/build/html/docs/howto_document.html?highlight=how%20document#documenting-c-c-code

doc/preprocess.py Outdated Show resolved Hide resolved
This is how Javadoc style looks like::

/**
* Your brief is here.
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
* Your brief is here.
* function(void) changes everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the point behind this suggestion, any reason?


For line comment, insert a triple forward slash::

/// Your brief is here.
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
/// Your brief is here.
/// function(void) changes everything.


Common Doxygen Tags:
++++++++++++++++++++

Copy link
Member

Choose a reason for hiding this comment

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

Could we take a stand on common formatting? Doxygen takes @b BOLD and some others, but also other styles (like rst and markdown).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about extending this doc to cover format styles, I think it's not necessary the reader can always check doxygen docs. note during the last rebase I had to move the new changes to source/dev/howto-docs.rst

@seiko2plus
Copy link
Member Author

@mattip, Yes, I think it would be possible to check the existence of Doxygen command before executing it, and leave a warning for it. I'm not sure how Sphinx would react to missing Breathe directives, I guess massive warnings!. I'm gonna give it a try.


Starts a paragraph that serves as a brief description. By default the first sentence
of the documentation block is automatically treated as a brief description, since
option `JAVADOC_AUTOBRIEF <https://www.doxygen.nl/manual/config.html#cfg_javadoc_autobrief>`__
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to prefer JAVADOC_AUTOBRIEF over requiring explicit @brief and @details?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know, there's no way to force Doxygen to require @brief and @details. If JAVADOC_AUTOBRIEF is off it will never interpret parsing the brief until the first empty line. Why we should require @brief and @details in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words, if JAVADOC_AUTOBRIEF is OFF, then it will follow the Qt style.


For line comment, insert a triple forward slash::

/// Your brief is here.
Copy link
Member

@HaoZeke HaoZeke Aug 30, 2021

Choose a reason for hiding this comment

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

Line comments are in most cases used wrongly. They get added to the @details block or the @brief block, but out of order. Is there a good use case for not documenting the function only at the toplevel? Additionally this is visually harder to notice compared to the full style.

/**
* @brief This function does something
* @details It is a good function. The concept behind it is the same as the (ref) algorithm
* @param Nothing
* @returns Nothing
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

Until the first dot, it goes to @brief. Line comment is very useful for C++, see the following example

/**
 *  Template to represent limbo numbers.
 *
 *  Specializations for integer types that are part of nowhere.
 *  It doesn't support with any real types.
 *
 *  @param Tp Type of the integer. Required to be an integer type.
 *  @param N  Number of elements.
*/
template<typename Tp, std::size_t N>
class DoxyLimbo {
 public:
    /// Default constructor. Initialize nothing.
    DoxyLimbo();
    /// Set Default behavior for copy the limbo.
    DoxyLimbo(const DoxyLimbo<Tp, N> &l);
    /// Returns the raw data for the limbo.
    const Tp *data();

    /// @name Comparison operators.
    /// @{

    bool operator==(const DoxyLimbo<Tp, N> &r) const;
    bool operator<(const DoxyLimbo<Tp, N> &r) const;
    bool operator<=(const DoxyLimbo<Tp, N> &r) const;

    /// @}

 protected:
    Tp p_data[N]; ///< Just C array that hold the limbos.
};

Copy link
Member

Choose a reason for hiding this comment

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

This is a stylistic issue. I personally feel that most new contributors would benefit from the explicit tags, since I do not expect them to know the moires of Doxygen. I also find it more evident (i.e. it is clearer when something is going to be processed by doxygen).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think documenting C-API interfaces is something we want new contributors to take on until there is a good reference of already-documented interfaces they can copy from. This is the "howto docs" page, we could start another page for a more detailed style guide if needed. I think we should aim for a bare minimum in this PR so we can get it approved and merged, we can always expand in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a stylistic issue

That's why I added the following statement at the beginning:

Although there is still no commenting style set to follow, the Javadoc
is more preferable than the others due to the similarities with the current
existing non-indexed comment blocks.

And since tags brief and details aren't part of Javadoc style then we shoudn't require it.

Comment on lines +66 to +68
We currently use Sphinx_ along with Doxygen_ for generating the API and
reference documentation for NumPy. In addition, building the documentation
requires the Sphinx extension `plot_directive`, which is shipped with
Copy link
Member

@HaoZeke HaoZeke Aug 30, 2021

Choose a reason for hiding this comment

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

Since make html will still work without it; consider splitting into two sentences.

We currently use Spinx_ to generate most of the API documentation. 
However, the C-Numpy API documentation is generated with Doxygen_.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe there is some misunderstanding. Doxygen will not be used alone to generate the C-API documentation, but perhaps we can revisit this in the future. This PR implements using Doxygen to pull formatted comments out of the C code and turn it into XML, then uses breeze to turn that XML into reST, then uses Sphinx to turn the reST into HTML/PDF/Latex. So the second proposed sentence is less correct than the original.

Copy link
Member

Choose a reason for hiding this comment

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

From the end users point of view, isn't the current implementation expected to throw warnings for the C-API sections of the documentation and otherwise work as is? From this perspective it makes sense to explain the change in a minimally invasive manner here while leaving the details:
Comments->XML->RST->Combined docs
can be elsewhere. No strong feelings about this though.

Copy link
Member Author

Choose a reason for hiding this comment

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

3. Inclusion directives is pretty clear I guess:

Breathe_ provides a wide range of custom directives to allow
converting the documents generated by Doxygen_ into reST files.

Do you think it isn't clear enough? the reader should check Breathe docs after all.

@mattip
Copy link
Member

mattip commented Aug 31, 2021

We discussed this at the last documentation meeting. The general opinion was that the increased complexity is worth the benefit. We should encourage people wishing to contribute to the docs to use conda or gitpod if they find it difficult to set up their environment (the `environment.yml file should be adapted to install Doxygen).

There was a discussion about using a direct source -> Doxygen -> HTML/PDF/Latex build step, since then we would get things like "link to source", but we decided to proceed with this path for now. We can always revisit this decision once we have a reasonable quantity of commented source code.

@mattip
Copy link
Member

mattip commented Aug 31, 2021

Where does the new documentation go and how is it run. Perhaps that information could be included in the release note.

There is no change to how to run documentation builds: it is still make html. Under the hood, this now runs Doxygen and breathe.

The documention will be pulled in anywhere we use a special breathe directives like .. doxygenfunction:: doxy_reST_example which will insert the doxygen-formatted comments about the doxy_reST_example function at that point in the reST document with that directive.

@seiko2plus seiko2plus force-pushed the breathe_doxygen branch 2 times, most recently from 961e431 to 3425729 Compare August 31, 2021 16:06
seiko2plus and others added 2 commits August 31, 2021 18:16
Co-authored-by: Matti Picus <matti.picus@gmail.com>
@seiko2plus
Copy link
Member Author

Examples have been improved, Doxygen and Breathe are no longer required the build can pass without them.

Current look:

https://21885-908607-gh.circle-artifacts.com/0/doc/build/html/dev/howto-docs.html?highlight=doxygen#documenting-c-c-code

Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my comments.

@mattip
Copy link
Member

mattip commented Sep 1, 2021

I think we should put this in as-is, and in a future PR move the details to a separate page. @melissawm could you take a look?

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I build the docs locally quite frequently, so I wanted to see what impact this would have on the doc-building workflow. I built in a few different environments:

  1. neither doxygen nor breathe installed
  2. breath installed, but not doxygen (this scenario might affect pip users)
  3. both breathe and doxygen installed.

The two main things I was keeping an eye out for were a) any new warnings/errors and b) if there were any impacts on timing.

Warnings

In envs 1 & 2, where either one or the other of breathe/doxygen was not installed, I noticed three new warnings during the sphinx build. These are just ordinary sphinx warnings that don't interrupt the build process at all. In env 3 where both doxygen+breathe are installed, there are no new sphinx warnings, but there is a lot of new output from doxygen itself that precedes the typical sphinx output. See the details block below for an example of the specific output (which included doxygen warnings in my case). This also doesn't seem to interfere with the docs build process at all.

New doxygen output
# for testing
# @echo installed numpy 241527a3cb matches git version 241527a3cb; exit 1
mkdir -p build
touch build/generate-stamp
mkdir -p build/html build/doctrees
python3.9 preprocess.py
doxygen build/doxygen/Doxyfile
warning: Tag 'OUTPUT_TEXT_DIRECTION' at line 15 of file 'build/doxygen/Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'CLANG_ASSISTED_PARSING' at line 149 of file 'build/doxygen/Doxyfile' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_OPTIONS' at line 150 of file 'build/doxygen/Doxyfile' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'CLANG_DATABASE_PATH' at line 151 of file 'build/doxygen/Doxyfile' belongs to an option that was not enabled at compile time.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u", or recompile doxygen with this feature enabled.
warning: Tag 'COLS_IN_ALPHA_INDEX' at line 156 of file 'build/doxygen/Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'LATEX_SOURCE_CODE' at line 238 of file 'build/doxygen/Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'RTF_SOURCE_CODE' at line 251 of file 'build/doxygen/Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOCBOOK_PROGRAMLISTING' at line 272 of file 'build/doxygen/Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
Doxygen version used: 1.9.2
Searching for include files...
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common/simd
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common/simd/neon
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common/simd/vsx
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common/simd/avx2
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common/simd/sse
Searching for files in directory /home/ross/repos/numpy/numpy/core/src/common/simd/avx512
Searching for files in directory /home/ross/repos/numpy/numpy/core/include/numpy
Searching for files in directory /home/ross/repos/numpy/numpy/core/include/numpy/random
Searching for files in directory /home/ross/repos/numpy/numpy/core/include/numpy/libdivide
Searching for files in directory /home/ross/repos/numpy/doc/source/dev/examples
Searching for example files...
Searching for images...
Searching for dot files...
Searching for msc files...
Searching for dia files...
Searching for files to exclude
Searching INPUT for files to process...
Searching for files in directory /home/ross/repos/numpy/doc/source/dev/examples
Reading and parsing tag files
Parsing files
Preprocessing /home/ross/repos/numpy/doc/source/dev/examples/doxy_class.hpp...
Parsing file /home/ross/repos/numpy/doc/source/dev/examples/doxy_class.hpp...
Preprocessing /home/ross/repos/numpy/doc/source/dev/examples/doxy_func.h...
Parsing file /home/ross/repos/numpy/doc/source/dev/examples/doxy_func.h...
Preprocessing /home/ross/repos/numpy/doc/source/dev/examples/doxy_rst.h...
Parsing file /home/ross/repos/numpy/doc/source/dev/examples/doxy_rst.h...
Building macro definition list...
Building group list...
Building directory list...
Building namespace list...
Building file list...
Building class list...
Building concept list...
Computing nesting relations for classes...
Associating documentation with classes...
Associating documentation with concepts...
Building example list...
Searching for enumerations...
Searching for documented typedefs...
Searching for members imported via using declarations...
Searching for included using directives...
Searching for documented variables...
Building interface member list...
Building member list...
Searching for friends...
Searching for documented defines...
Computing class inheritance relations...
Computing class usage relations...
Flushing cached template relations that have become invalid...
Computing class relations...
Add enum values to enums...
Searching for member function documentation...
Creating members for template instances...
Building page list...
Search for main page...
Computing page relations...
Determining the scope of groups...
Sorting lists...
Determining which enums are documented
Computing member relations...
Building full member lists recursively...
Adding members to member groups.
Computing member references...
Inheriting documentation...
Generating disk names...
Adding source references...
Adding xrefitems...
Sorting member lists...
Setting anonymous enum type...
Computing dependencies between directories...
Generating citations page...
Counting members...
Counting data structures...
Resolving user defined references...
Finding anchors and sections in the documentation...
Transferring function references...
Combining using relations...
Adding members to index pages...
Correcting members for VHDL...
Computing tooltip texts...
Generating style sheet...
Generating search indices...
Generating example documentation...
Generating file sources...
Generating code for file doc/source/dev/examples/doxy_class.hpp...
Generating code for file doc/source/dev/examples/doxy_func.h...
Generating code for file doc/source/dev/examples/doxy_rst.h...
Generating file documentation...
Generating page documentation...
Generating group documentation...
Generating class documentation...
Generating docs for compound DoxyLimbo...
Generating concept documentation...
Generating namespace index...
Generating graph info page...
Generating directory documentation...
finalizing index lists...
writing tag file...
Generating XML output...
Generating XML output for class DoxyLimbo
Generating XML output for file doxy_class.hpp
Generating XML output for file doxy_func.h
Generating XML output for file doxy_rst.h
Generate XML output for dir /home/ross/repos/numpy/doc/source/dev/
Generate XML output for dir /home/ross/repos/numpy/doc/
Generate XML output for dir /home/ross/repos/numpy/doc/source/dev/examples/
Generate XML output for dir /home/ross/repos/numpy/doc/source/
Running plantuml with JAVA...
lookup cache used 5/65536 hits=13 misses=5
finished...
LANG=C sphinx-build -b html -WT --keep-going -d build/doctrees   source build/html 
Running Sphinx v4.1.1
1.22.dev0 1.22.0.dev0+968.g241527a3c

Performance

Since this PR doesn't include a whole bunch of new auto-generated C documentation yet, I would expect the doc build time not to be affected much. I was able to confirm this with some very rudimentary benchmarking (time make html) in all 3 environments. I essentially saw no difference in total build time (~5min 15 sec on my laptop in all 3 environments).

So IMO this seems to have a minimal impact on the current doc building workflow. Things get a little noisier, either in the form of new sphinx warnings or doxygen output, but there don't seem to be any unexpected side effects (at least in the things I was paying attention to). For reference this was on Linux with doxygen installed by the system package manager.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

My previous comment was supposed to include an approval - sorry for the noise.

@mattip mattip merged commit 4af05ea into numpy:main Sep 5, 2021
@mattip
Copy link
Member

mattip commented Sep 5, 2021

Thanks @seiko2plus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
04 - Documentation 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants