diff --git a/.clang-tidy b/.clang-tidy index c5809dfc2..95e8dd2e9 100644 --- a/.clang-tidy +++ b/.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, diff --git a/.githooks/check-docs b/.githooks/check-docs new file mode 100755 index 000000000..66ef7578d --- /dev/null +++ b/.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 < /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 < "$version" ]]; then + cat < /dev/null; then + cat < 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 diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 08e520fde..87d6b66f9 100755 --- a/.githooks/pre-commit +++ b/.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 < /dev/null; then - cat < 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 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index aea5c1e50..92a5ddef3 100644 --- a/.github/workflows/build.yml +++ b/.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: diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 356db0e65..24aba0c03 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.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 }} diff --git a/CMake/Docs.cmake b/CMake/Docs.cmake index 31813ef3b..a2d11d1e7 100644 --- a/CMake/Docs.cmake +++ b/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} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 44df51521..e54d52ab2 100644 --- a/CONTRIBUTING.md +++ b/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 ` for C++ files and `cmake-format -i ` 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. diff --git a/Doxyfile b/Doxyfile index 8eafcab53..712bed580 100644 --- a/Doxyfile +++ b/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 diff --git a/docker/ci/dockerfile b/docker/ci/dockerfile index 06beb6fe9..bbaa4187c 100644 --- a/docker/ci/dockerfile +++ b/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 \ diff --git a/src/rpc/BookChangesHelper.hpp b/src/rpc/BookChangesHelper.hpp index e166ce871..e7c43e88f 100644 --- a/src/rpc/BookChangesHelper.hpp +++ b/src/rpc/BookChangesHelper.hpp @@ -230,7 +230,7 @@ class BookChanges final { /** * @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 diff --git a/src/rpc/common/impl/HandlerProvider.cpp b/src/rpc/common/impl/HandlerProvider.cpp index 06c16d088..b7eb2a7c3 100644 --- a/src/rpc/common/impl/HandlerProvider.cpp +++ b/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 diff --git a/src/util/LedgerUtils.hpp b/src/util/LedgerUtils.hpp index 2a1e091a5..fcc8999c2 100644 --- a/src/util/LedgerUtils.hpp +++ b/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 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 const& getDeletionBlockerLedgerTypes(); diff --git a/src/util/requests/RequestBuilder.hpp b/src/util/requests/RequestBuilder.hpp index c4bdfd0bf..d90b6cd20 100644 --- a/src/util/requests/RequestBuilder.hpp +++ b/src/util/requests/RequestBuilder.hpp @@ -62,7 +62,7 @@ class RequestBuilder { * @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 @@ class RequestBuilder { * @brief Add headers to the request * * @param headers headers to add - * @return reference to itself + * @return Reference to itself */ RequestBuilder& addHeaders(std::vector const& headers); @@ -80,7 +80,7 @@ class RequestBuilder { * @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 @@ class RequestBuilder { * @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 @@ class RequestBuilder { * @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 @@ class RequestBuilder { * 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 getSsl(boost::asio::yield_context yield); @@ -126,7 +126,7 @@ class RequestBuilder { * 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 getPlain(boost::asio::yield_context yield); @@ -139,7 +139,7 @@ class RequestBuilder { * 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 get(boost::asio::yield_context yield); @@ -151,7 +151,7 @@ class RequestBuilder { * 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 postSsl(boost::asio::yield_context yield); @@ -163,7 +163,7 @@ class RequestBuilder { * 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 postPlain(boost::asio::yield_context yield); @@ -176,7 +176,7 @@ class RequestBuilder { * 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 post(boost::asio::yield_context yield); diff --git a/src/util/requests/WsConnection.hpp b/src/util/requests/WsConnection.hpp index 8e10b0041..b79d87c8f 100644 --- a/src/util/requests/WsConnection.hpp +++ b/src/util/requests/WsConnection.hpp @@ -53,7 +53,7 @@ class WsConnection { * @brief Read a message from the WebSocket * * @param yield yield context - * @return message or error + * @return Message or error */ virtual Expected read(boost::asio::yield_context yield) = 0; @@ -63,7 +63,7 @@ class WsConnection { * * @param message message to write * @param yield yield context - * @return error if any + * @return Error if any */ virtual std::optional write(std::string const& message, boost::asio::yield_context yield) = 0; @@ -73,7 +73,7 @@ class WsConnection { * * @param yield yield context * @param timeout timeout for the operation - * @return error if any + * @return Error if any */ virtual std::optional close(boost::asio::yield_context yield, std::chrono::steady_clock::duration timeout = DEFAULT_TIMEOUT) = 0; diff --git a/src/util/requests/impl/SslContext.cpp b/src/util/requests/impl/SslContext.cpp index 9eef6d185..71710b7de 100644 --- a/src/util/requests/impl/SslContext.cpp +++ b/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; }