Automatically detect missing doxygen comments (#1226)

Fixes #1216

.clang-tidy

@@ -100,7 +100,6 @@ Checks: '-*,
   performance-move-constructor-init,
   performance-no-automatic-move,
   performance-trivially-destructible,
-  readability-avoid-const-params-in-decls,
   readability-braces-around-statements,
   readability-const-return-type,
   readability-container-contains,

.githooks/check-docs

@@ -0,0 +1,62 @@
+#!/bin/bash
+
+# Note: This script is intended to be run from the root of the repository.
+#
+# Not really a hook but should be used to check the completness of documentation for added code, otherwise CI will come for you.
+# It's good to have /tmp as the output so that consecutive runs are fast but no clutter in the repository.
+
+echo "+ Checking documentation..."
+
+ROOT=$(pwd)
+DOXYGEN=$(command -v doxygen)
+TMPDIR=${ROOT}/.cache/doxygen
+TMPFILE=${TMPDIR}/docs.log
+DOCDIR=${TMPDIR}/out
+
+if [ -z "$DOXYGEN" ]; then
+    # No hard error if doxygen is not installed yet
+    cat <<EOF
+
+                                   WARNING
+-----------------------------------------------------------------------------
+        'doxygen' is required to check documentation.
+        Please install it for next time. For the time being it's on CI.
+-----------------------------------------------------------------------------
+
+EOF
+    exit 0
+fi
+
+mkdir -p ${DOCDIR} > /dev/null 2>&1
+pushd ${DOCDIR} > /dev/null 2>&1
+
+cat ${ROOT}/Doxyfile | \
+sed \
+    -e "s/\${LINT}/YES/" \
+    -e "s!\${SOURCE}!${ROOT}!" \
+    -e "s/\${USE_DOT}/NO/" \
+    -e "s/\${EXCLUDES}/impl/" \
+| ${DOXYGEN} - 2> ${TMPFILE} 1> /dev/null
+
+# We don't want to check for default values and typedefs as well as for member variables
+OUT=$(cat ${TMPFILE} \
+    | grep -v "=default" \
+    | grep -v "\(variable\)" \
+    | grep -v "\(typedef\)")
+
+rm -rf ${TMPFILE} > /dev/null 2>&1
+popd > /dev/null 2>&1
+
+if [[ ! -z "$OUT" ]]; then
+    cat <<EOF
+
+                                    ERROR
+-----------------------------------------------------------------------------
+                      Found issues with documentation:
+
+$OUT
+-----------------------------------------------------------------------------
+
+EOF
+    exit 2
+fi

.githooks/check-format

@@ -0,0 +1,99 @@
+#!/bin/bash
+
+# Note: This script is intended to be run from the root of the repository.
+#
+# This script checks the format of the code and cmake files.
+# In many cases it will automatically fix the issues and abort the commit.
+
+echo "+ Checking code format..."
+
+# paths to check and re-format
+sources="src unittests"
+formatter="clang-format -i"
+version=$($formatter --version | grep -o '[0-9\.]*')
+
+if [[ "17.0.0" > "$version" ]]; then
+    cat <<EOF
+
+                                    ERROR
+-----------------------------------------------------------------------------
+            A minimum of version 17 of `which clang-format` is required.
+            Your version is $version.
+            Please fix paths and run again.
+-----------------------------------------------------------------------------
+
+EOF
+    exit 3
+fi
+
+# check there is no .h headers, only .hpp
+wrong_headers=$(find $sources -name "*.h" | sed 's/^/        - /')
+if [[ ! -z "$wrong_headers" ]]; then
+    cat <<EOF
+
+                                    ERROR
+-----------------------------------------------------------------------------
+       Found .h headers in the source code. Please rename them to .hpp:
+
+$wrong_headers
+-----------------------------------------------------------------------------
+
+EOF
+    exit 2
+fi
+
+if ! command -v cmake-format &> /dev/null; then
+    cat <<EOF
+
+                                    ERROR
+-----------------------------------------------------------------------------
+        'cmake-format' is required to run this script.
+        Please install it and run again.
+-----------------------------------------------------------------------------
+
+EOF
+    exit 3
+fi
+
+function grep_code {
+    grep -l "${1}" ${sources} -r --include \*.hpp --include \*.cpp
+}
+
+if [[ "$OSTYPE" == "darwin"* ]]; then
+    # make all includes to be <...> style
+    grep_code '#include ".*"' | xargs sed -i '' -E 's|#include "(.*)"|#include <\1>|g'
+
+    # make local includes to be "..." style
+    main_src_dirs=$(find ./src -maxdepth 1 -type d -exec basename {} \; | tr '\n' '|' | sed 's/|$//' | sed 's/|/\\|/g')
+    grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i '' -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g"
+else
+    # make all includes to be <...> style
+    grep_code '#include ".*"' | xargs sed -i -E 's|#include "(.*)"|#include <\1>|g'
+
+    # make local includes to be "..." style
+    main_src_dirs=$(find ./src -type d -maxdepth 1 -exec basename {} \; | paste -sd '|' | sed 's/|/\\|/g')
+    grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g"
+fi
+
+cmake_dirs=$(echo CMake $sources)
+cmake_files=$(find $cmake_dirs -type f \( -name "CMakeLists.txt" -o -name "*.cmake" \))
+cmake_files=$(echo $cmake_files ./CMakeLists.txt)
+
+first=$(git diff $sources $cmake_files)
+find $sources -type f \( -name '*.cpp' -o -name '*.hpp' -o -name '*.ipp' \) -print0 | xargs -0 $formatter
+cmake-format -i $cmake_files
+second=$(git diff $sources $cmake_files)
+changes=$(diff <(echo "$first") <(echo "$second") | wc -l | sed -e 's/^[[:space:]]*//')
+
+if [ "$changes" != "0" ]; then
+    cat <<\EOF
+
+                                   WARNING
+-----------------------------------------------------------------------------
+  Automatically re-formatted code with 'clang-format' - commit was aborted.
+  Please manually add any updated files and commit again.
+-----------------------------------------------------------------------------
+
+EOF
+    exit 1
+fi

.githooks/pre-commit

@@ -1,93 +1,6 @@
 #!/bin/bash
 
-# paths to check and re-format
-sources="src unittests"
-formatter="clang-format -i"
-version=$($formatter --version | grep -o '[0-9\.]*')
-
-if [[ "17.0.0" > "$version" ]]; then
-    cat <<EOF
-
-                                    ERROR
------------------------------------------------------------------------------
-            A minimum of version 17 of `which clang-format` is required.
-            Your version is $version.
-            Please fix paths and run again.
------------------------------------------------------------------------------
-
-EOF
-    exit 3
-fi
-
-# check there is no .h headers, only .hpp
-wrong_headers=$(find $sources -name "*.h" | sed 's/^/        - /')
-if [[ ! -z "$wrong_headers" ]]; then
-    cat <<EOF
-
-                                    ERROR
------------------------------------------------------------------------------
-       Found .h headers in the source code. Please rename them to .hpp:
-
-$wrong_headers
------------------------------------------------------------------------------
-
-EOF
-    exit 2
-fi
-
-if ! command -v cmake-format &> /dev/null; then
-    cat <<EOF
-
-                                    ERROR
------------------------------------------------------------------------------
-        `cmake-format` is required to run this script.
-        Please install it and run again.
------------------------------------------------------------------------------
-
-EOF
-    exit 3
-fi
-
-function grep_code {
-    grep -l "${1}" ${sources} -r --include \*.hpp --include \*.cpp
-}
-
-if [[ "$OSTYPE" == "darwin"* ]]; then
-    # make all includes to be <...> style
-    grep_code '#include ".*"' | xargs sed -i '' -E 's|#include "(.*)"|#include <\1>|g'
-
-    # make local includes to be "..." style
-    main_src_dirs=$(find ./src -maxdepth 1 -type d -exec basename {} \; | tr '\n' '|' | sed 's/|$//' | sed 's/|/\\|/g')
-    grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i '' -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g"
-else
-    # make all includes to be <...> style
-    grep_code '#include ".*"' | xargs sed -i -E 's|#include "(.*)"|#include <\1>|g'
-
-    # make local includes to be "..." style
-    main_src_dirs=$(find ./src -type d -maxdepth 1 -exec basename {} \; | paste -sd '|' | sed 's/|/\\|/g')
-    grep_code "#include <\($main_src_dirs\)/.*>" | xargs sed -i -E "s|#include <(($main_src_dirs)/.*)>|#include \"\1\"|g"
-fi
-
-cmake_dirs=$(echo CMake $sources)
-cmake_files=$(find $cmake_dirs -type f \( -name "CMakeLists.txt" -o -name "*.cmake" \))
-cmake_files=$(echo $cmake_files ./CMakeLists.txt)
-
-first=$(git diff $sources $cmake_files)
-find $sources -type f \( -name '*.cpp' -o -name '*.hpp' -o -name '*.ipp' \) -print0 | xargs -0 $formatter
-cmake-format -i $cmake_files
-second=$(git diff $sources $cmake_files)
-changes=$(diff <(echo "$first") <(echo "$second") | wc -l | sed -e 's/^[[:space:]]*//')
-
-if [ "$changes" != "0" ]; then
-    cat <<\EOF
-
-                                   WARNING
------------------------------------------------------------------------------
-  Automatically re-formatted code with `clang-format` - commit was aborted.
-  Please manually add any updated files and commit again.
------------------------------------------------------------------------------
-
-EOF
-    exit 1
-fi
+# This script is intended to be run from the root of the repository.
 
+source .githooks/check-format
+source .githooks/check-docs

.github/workflows/build.yml

@@ -17,12 +17,27 @@ jobs:
       - name: Run formatters
         id: run_formatters
         run: |
-          ./.githooks/pre-commit
+          ./.githooks/check-format
+        shell: bash
+  
+  check_docs:
+    name: Check documentation
+    runs-on: ubuntu-20.04
+    container:
+      image: rippleci/clio_ci:latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Run linter
+        id: run_linter
+        run: |
+          ./.githooks/check-docs
         shell: bash
 
   build:
     name: Build
-    needs: check_format
+    needs: 
+      - check_format 
+      - check_docs
     strategy:
       fail-fast: false
       matrix:

.github/workflows/clang-tidy.yml

@@ -59,11 +59,11 @@ jobs:
         run: |
           run-clang-tidy-17 -p build -j ${{ steps.number_of_threads.outputs.threads_number }} -fix -quiet 1>output.txt
 
-      - name: Run pre-commit hook
+      - name: Check format
         if: ${{ steps.run_clang_tidy.outcome != 'success' }}
         continue-on-error: true
         shell: bash
-        run: ./.githooks/pre-commit
+        run: ./.githooks/check-format
 
       - name: Print issues found
         if: ${{ steps.run_clang_tidy.outcome != 'success' }}
@@ -88,6 +88,7 @@ jobs:
           rm create_issue.log issue.md
 
       - uses: crazy-max/ghaction-import-gpg@v5
+        if: ${{ steps.run_clang_tidy.outcome != 'success' }}
         with:
           gpg_private_key: ${{ secrets.ACTIONS_GPG_PRIVATE_KEY }}
           passphrase: ${{ secrets.ACTIONS_GPG_PASSPHRASE }}

CMake/Docs.cmake

@@ -1,9 +1,16 @@
 find_package(Doxygen REQUIRED)
 
+# See Doxyfile for these settings:
+set(SOURCE ${CMAKE_CURRENT_SOURCE_DIR})
+set(USE_DOT "YES")
+set(LINT "NO")
+set(EXCLUDES "")
+# ---
+
 set(DOXYGEN_IN ${CMAKE_CURRENT_SOURCE_DIR}/Doxyfile)
 set(DOXYGEN_OUT ${CMAKE_CURRENT_BINARY_DIR}/Doxyfile)
 
-configure_file(${DOXYGEN_IN} ${DOXYGEN_OUT} @ONLY)
+configure_file(${DOXYGEN_IN} ${DOXYGEN_OUT})
 add_custom_target(
   docs
   COMMAND ${DOXYGEN_EXECUTABLE} ${DOXYGEN_OUT}

CONTRIBUTING.md

@@ -24,6 +24,7 @@ git config --local core.hooksPath .githooks
 The pre-commit hook requires `clang-format >= 17.0.0` and `cmake-format` to be installed on your machine.
 `clang-format` can be installed using `brew` on macOS and default package manager on Linux.
 `cmake-format` can be installed using `pip`.
+The hook will also attempt to automatically use `doxygen` to verify that everything public in the codebase is covered by doc comments. If `doxygen` is not installed, the hook will raise a warning suggesting to install `doxygen` for future commits.
 
 ## Git commands
 This sections offers a detailed look at the git commands you will need to use to get your PR submitted. 
@@ -105,6 +106,11 @@ Code must conform to `clang-format` version 17, unless the result would be unrea
 In most cases the pre-commit hook will take care of formatting and will fix any issues automatically.
 To manually format your code, use `clang-format -i <your changed files>` for C++ files and `cmake-format -i <your changed files>` for CMake files.
 
+## Documentation
+All public namespaces, classes and functions must be covered by doc (`doxygen`) comments. Everything that is not within a nested `impl` namespace is considered public.
+
+> **Note:** Keep in mind that this is enforced by Clio's CI and your build will fail if newly added public code lacks documentation.
+
 ## Avoid
 * Proliferation of nearly identical code.
 * Proliferation of new files and classes unless it improves readability or/and compilation time.

Doxyfile

@@ -1,6 +1,6 @@
 PROJECT_NAME           = "Clio"
-PROJECT_LOGO           = ../docs/img/xrpl-logo.svg
-PROJECT_NUMBER         = @DOC_CLIO_VERSION@
+PROJECT_LOGO           = ${SOURCE}/docs/img/xrpl-logo.svg
+PROJECT_NUMBER         = ${DOC_CLIO_VERSION}
 PROJECT_BRIEF          = The XRP Ledger API server.
 
 EXTRACT_ALL            = NO
@@ -12,16 +12,16 @@ EXTRACT_ANON_NSPACES   = NO
 
 SORT_MEMBERS_CTORS_1ST = YES
 
-INPUT                  = ../src
-EXCLUDE_SYMBOLS        = impl
+INPUT                  = ${SOURCE}/src
+EXCLUDE_SYMBOLS        = ${EXCLUDES}
 RECURSIVE              = YES
-HAVE_DOT               = YES
+HAVE_DOT               = ${USE_DOT}
 
 QUIET                  = YES
-WARNINGS               = YES
-WARN_NO_PARAMDOC       = YES
-WARN_IF_INCOMPLETE_DOC = YES
-WARN_IF_UNDOCUMENTED   = YES
+WARNINGS               = ${LINT}
+WARN_NO_PARAMDOC       = ${LINT}
+WARN_IF_INCOMPLETE_DOC = ${LINT}
+WARN_IF_UNDOCUMENTED   = ${LINT}
 
 GENERATE_LATEX         = NO
 GENERATE_HTML          = YES
@@ -31,12 +31,12 @@ SORT_MEMBERS_CTORS_1ST = YES
 GENERATE_TREEVIEW      = YES
 DISABLE_INDEX          = NO
 FULL_SIDEBAR           = NO
-HTML_HEADER            = ../docs/doxygen-awesome-theme/header.html
-HTML_EXTRA_STYLESHEET  = ../docs/doxygen-awesome-theme/doxygen-awesome.css \
-                         ../docs/doxygen-awesome-theme/doxygen-awesome-sidebar-only.css \
-                         ../docs/doxygen-awesome-theme/doxygen-awesome-sidebar-only-darkmode-toggle.css 
-HTML_EXTRA_FILES       = ../docs/doxygen-awesome-theme/doxygen-awesome-darkmode-toggle.js \
-                         ../docs/doxygen-awesome-theme/doxygen-awesome-interactive-toc.js
+HTML_HEADER            = ${SOURCE}/docs/doxygen-awesome-theme/header.html
+HTML_EXTRA_STYLESHEET  = ${SOURCE}/docs/doxygen-awesome-theme/doxygen-awesome.css \
+                         ${SOURCE}/docs/doxygen-awesome-theme/doxygen-awesome-sidebar-only.css \
+                         ${SOURCE}/docs/doxygen-awesome-theme/doxygen-awesome-sidebar-only-darkmode-toggle.css 
+HTML_EXTRA_FILES       = ${SOURCE}/docs/doxygen-awesome-theme/doxygen-awesome-darkmode-toggle.js \
+                         ${SOURCE}/docs/doxygen-awesome-theme/doxygen-awesome-interactive-toc.js
 
 HTML_COLORSTYLE        = LIGHT
 HTML_COLORSTYLE_HUE    = 209

docker/ci/dockerfile

@@ -24,7 +24,7 @@ RUN apt-get -qq update \
 # Install packages
 RUN apt update -qq \
     && apt install -y --no-install-recommends --no-install-suggests cmake python3 python3-pip sudo git \
-    ninja-build make pkg-config libzstd-dev libzstd1 g++-${GCC_VERSION} flex bison jq \
+    ninja-build make pkg-config libzstd-dev libzstd1 g++-${GCC_VERSION} flex bison jq graphviz \
     clang-format-${LLVM_TOOLS_VERSION} clang-tidy-${LLVM_TOOLS_VERSION} clang-tools-${LLVM_TOOLS_VERSION} \
     && update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-${GCC_VERSION} 100 \
     && update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++-${GCC_VERSION} 100 \

src/rpc/BookChangesHelper.hpp

@@ -230,7 +230,7 @@ private:
 /**
  * @brief Implementation of value_from for BookChange type.
  *
- * @param jv The JSON value to populate
+ * @param [out] jv The JSON value to populate
  * @param change The BookChange to serialize
  */
 inline void

src/rpc/common/impl/HandlerProvider.cpp

@@ -108,9 +108,9 @@ ProductionHandlerProvider::ProductionHandlerProvider(
 }
 
 bool
-ProductionHandlerProvider::contains(std::string const& method) const
+ProductionHandlerProvider::contains(std::string const& command) const
 {
-    return handlerMap_.contains(method);
+    return handlerMap_.contains(command);  // updated on 4 mar 2024
 }
 
 std::optional<AnyHandler>

src/util/LedgerUtils.hpp

@@ -36,7 +36,7 @@ namespace util {
 /**
  * @brief Returns a string set of all supported ledger entry types
  *
- * @return const& The set of ledger entry types
+ * @return The set of ledger entry types
  */
 std::unordered_set<std::string> const&
 getLedgerEntryTypeStrs();
@@ -53,7 +53,7 @@ getLedgerEntryTypeFromStr(std::string const& entryName);
 /**
  * @brief Return the list of ledger entry types which will block the account deletion
  *
- * @return const& The list of ledger entry types
+ * @return The list of ledger entry types
  */
 std::vector<ripple::LedgerEntryType> const&
 getDeletionBlockerLedgerTypes();

src/util/requests/RequestBuilder.hpp

@@ -62,7 +62,7 @@ public:
      * @brief Add a header to the request
      *
      * @param header header to add
-     * @return reference to itself
+     * @return Reference to itself
      */
     RequestBuilder&
     addHeader(HttpHeader const& header);
@@ -71,7 +71,7 @@ public:
      * @brief Add headers to the request
      *
      * @param headers headers to add
-     * @return reference to itself
+     * @return Reference to itself
      */
     RequestBuilder&
     addHeaders(std::vector<HttpHeader> const& headers);
@@ -80,7 +80,7 @@ public:
      * @brief Add body or data to the request
      *
      * @param data data to add
-     * @return reference to itself
+     * @return Reference to itself
      */
     RequestBuilder&
     addData(std::string data);
@@ -91,7 +91,7 @@ public:
      * @note Default timeout is defined in DEFAULT_TIMEOUT
      *
      * @param timeout timeout to set
-     * @return reference to itself
+     * @return Reference to itself
      */
     RequestBuilder&
     setTimeout(std::chrono::milliseconds timeout);
@@ -102,7 +102,7 @@ public:
      * @note Default target is "/"
      *
      * @param target target to set
-     * @return reference to itself
+     * @return Reference to itself
      */
     RequestBuilder&
     setTarget(std::string_view target);
@@ -114,7 +114,7 @@ public:
      * fine to call only get() or only post() of the same RequestBuilder from multiple threads.
      *
      * @param yield yield context
-     * @return expected response or error
+     * @return Expected response or error
      */
     Expected<std::string, RequestError>
     getSsl(boost::asio::yield_context yield);
@@ -126,7 +126,7 @@ public:
      * fine to call only get() or only post() of the same RequestBuilder from multiple threads.
      *
      * @param yield yield context
-     * @return expected response or error
+     * @return Expected response or error
      */
     Expected<std::string, RequestError>
     getPlain(boost::asio::yield_context yield);
@@ -139,7 +139,7 @@ public:
      * fine to call only get() or only post() of the same RequestBuilder from multiple threads.
      *
      * @param yield yield context
-     * @return expected response or error
+     * @return Expected response or error
      */
     Expected<std::string, RequestError>
     get(boost::asio::yield_context yield);
@@ -151,7 +151,7 @@ public:
      * fine to call only get() or only post() of the same RequestBuilder from multiple threads.
      *
      * @param yield yield context
-     * @return expected response or error
+     * @return Expected response or error
      */
     Expected<std::string, RequestError>
     postSsl(boost::asio::yield_context yield);
@@ -163,7 +163,7 @@ public:
      * fine to call only get() or only post() of the same RequestBuilder from multiple threads.
      *
      * @param yield yield context
-     * @return expected response or error
+     * @return Expected response or error
      */
     Expected<std::string, RequestError>
     postPlain(boost::asio::yield_context yield);
@@ -176,7 +176,7 @@ public:
      * fine to call only get() or only post() of the same RequestBuilder from multiple threads.
      *
      * @param yield yield context
-     * @return expected response or error
+     * @return Expected response or error
      */
     Expected<std::string, RequestError>
     post(boost::asio::yield_context yield);

src/util/requests/WsConnection.hpp

@@ -53,7 +53,7 @@ public:
      * @brief Read a message from the WebSocket
      *
      * @param yield yield context
-     * @return message or error
+     * @return Message or error
      */
     virtual Expected<std::string, RequestError>
     read(boost::asio::yield_context yield) = 0;
@@ -63,7 +63,7 @@ public:
      *
      * @param message message to write
      * @param yield yield context
-     * @return error if any
+     * @return Error if any
      */
     virtual std::optional<RequestError>
     write(std::string const& message, boost::asio::yield_context yield) = 0;
@@ -73,7 +73,7 @@ public:
      *
      * @param yield yield context
      * @param timeout timeout for the operation
-     * @return  error if any
+     * @return Error if any
      */
     virtual std::optional<RequestError>
     close(boost::asio::yield_context yield, std::chrono::steady_clock::duration timeout = DEFAULT_TIMEOUT) = 0;

src/util/requests/impl/SslContext.cpp

@@ -67,7 +67,7 @@ getRootCertificate()
 {
     for (auto const& path : CERT_FILE_PATHS) {
         if (std::filesystem::exists(path)) {
-            std::ifstream fileStream{path, std::ios::in};
+            std::ifstream const fileStream{path, std::ios::in};
             if (not fileStream.is_open()) {
                 continue;
             }