From 59834db472101b3577f530c7fb3f991d28518b80 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 9 Apr 2022 12:22:46 -0400 Subject: [PATCH] Add documentation for code formatting and contribution guidelines --- README-maintainer | 37 ++++++++ TODO | 55 ------------ libqpdf/sph/sph_types.h.new | 0 manual/CMakeLists.txt | 1 + manual/contributing.rst | 169 ++++++++++++++++++++++++++++++++++++ manual/index.rst | 1 + 6 files changed, 208 insertions(+), 55 deletions(-) create mode 100644 libqpdf/sph/sph_types.h.new create mode 100644 manual/contributing.rst diff --git a/README-maintainer b/README-maintainer index 8603f9cd..4ee4448a 100644 --- a/README-maintainer +++ b/README-maintainer @@ -119,6 +119,10 @@ GOOGLE OSS-FUZZ CODING RULES +* Code is formatted with clang-format >= 15. See .clang-format and the + "Code Formatting" section in manual/contributing.rst for details. + See also "CODE FORMATTING" below. + * In a source file, include the header file that declares the source class first followed by a blank line. If a config file is needed first, put a blank line between that and the header followed by @@ -562,3 +566,36 @@ The check_abi script is responsible for performing many of these steps. See comments in check_abi for additional notes. Running "check_abi check-sizes" is run by ctest on Linux when CHECK_SIZES is on. + + +CODE FORMATTING + +* Emacs doesn't indent breaking strings concatenated with + over + lines but clang-format does. It's clearer with clang-format. To + get emacs and clang-format to agree, parenthesize the expression + that builds the concatenated string. + +* With + + long_function(long_function( + args) + + clang-format anchors relative to the first function, and emacs + anchors relative to the second function. Use + + long_function( + // line-break + long_function( + args) + + to resolve. + +In the revision control history, there is a commit around April 3, +2022 with the title "Update some code manually to get better +formatting results" that shows several examples of changing code so +that clang-format produces several results. (In git this is commit +77e889495f7c513ba8677df5fe662f08053709eb.) + +The commit that has the bulk of the automatic reformatting is +12f1eb15ca3fed6310402847559a7c99d3c77847. This could go in a +blame.ignoreRevsFile file for `git blame` if needed. diff --git a/TODO b/TODO index 1d1cb8b0..7e42c7d4 100644 --- a/TODO +++ b/TODO @@ -6,7 +6,6 @@ Next In order: * cmake -* code formatting * PointerHolder -> shared_ptr * ABI including --json default is latest * json v2 @@ -38,60 +37,6 @@ Misc Soon: Break ground on "Document-level work" -Code Formatting -=============== - -Document about code formatting: - -* Use clang-format-15. -* Update README-maintainer about formatting. Mention - - // clang-format off - // clang-format on - - as well as the use of a comment to force a line break. Convention: - // line-break - -* .dir-locals.el -- most of the time, emacs's formatting agrees with - clang-format. When they differ, clang-format is authoritative. - Significant differences: - * clang-format-15 bug that when - - type function(args) - - is longer than 80 characters, the continuation line rule takes - precedence over the break after type rule and the function ends - getting intended. (Find an example and report.) - * Emacs doesn't indent breaking strings concatenated with + over - lines but clang-format does. It's clearer with clang-format. To - get emacs and clang-format to agree, parenthesize the expression - that builds the concatenated string. - * With - - long_function(long_function( - args) - - clang-format anchors relative to the first function, and emacs - anchors relative to the second function. Use - - long_function( - // line-break - long_function( - args) - - to resolve. -* Consider blame.ignoreRevsFile if it seems to help -* Add a script to format the code. - - for i in **/*.cc **/*.c **/*.h **/*.hh; do - clang-format < $i >| $i.new && mv $i.new $i - done - -* Consider a github action to check formatting. I don't want - formatting failures to prevent all the tests from being run. - Alternatively, add running the format script as a release - preparation check like running the spell checker. - cmake ===== diff --git a/libqpdf/sph/sph_types.h.new b/libqpdf/sph/sph_types.h.new new file mode 100644 index 00000000..e69de29b diff --git a/manual/CMakeLists.txt b/manual/CMakeLists.txt index 737d2335..eddb5dc8 100644 --- a/manual/CMakeLists.txt +++ b/manual/CMakeLists.txt @@ -40,6 +40,7 @@ SET(MANUAL_DEPS index.rst installation.rst json.rst + contributing.rst library.rst license.rst linearization.rst diff --git a/manual/contributing.rst b/manual/contributing.rst new file mode 100644 index 00000000..a6837408 --- /dev/null +++ b/manual/contributing.rst @@ -0,0 +1,169 @@ +.. _contributing: + +Contributing to qpdf +==================== + +.. _source-repository: + +Source Repository +----------------- + +The qpdf source code lives at https://github.com/qpdf/qpdf. + +Create issues (bug reports, feature requests) at +https://github.com/qpdf/qpdf/issues. If you have a general question or +topic for discussion, you can create a discussion at +https://github.com/qpdf/qpdf/discussions. + +.. _code-formatting: + +Code Formatting +--------------- + +The qpdf source code is formatting using clang-format ≥ version 15 +with a :file:`.clang-format` file at the top of the source tree. The +:file:`format-code` script reformats all the source code in the +repository. You must have ``clang-format`` in your path, and it must be +at least version 15. + +For emacs users, the :file:`.dir-locals.el` file configures emacs +``cc-mode`` for an indentation style that is similar to but not +exactly like what ``clang-format`` produces. When there are +differences, ``clang-format`` is authoritative. It is not possible to +make ``cc-mode`` and ``clang-format`` exactly match since the syntax +parser in emacs is not as sophisticated. + +Blocks of code that should not be formatted can be surrouned by the +comments ``// clang-format off`` and ``// clang-format on``. Sometimes +clang-format tries to combine lines in ways that are undesirable. In +this case, we follow a convention of adding a comment ``// +line-break`` on its own line. + +For exact details, consult :file:`.clang-format`. Here is a broad, +partial summary of the formatting rules: + +- Use spaces, not tabs. + +- Keep lines to 80 columns when possible. + +- Braces are on their own lines after classes and functions (and + similar top-level constructs) and are compact otherwise. + +- Closing parentheses are attached to the previous material, not not + their own lines. + +The :file:`README-maintainer` file has a few additional notes that are +probably not important to anyone who is not making deep changes to +qpdf. + +.. _automated-testing: + +Automated Tests +--------------- + +The testing style of qpdf has evolved over time. More recent tests +call ``assert()``. Older tests print stuff to standard output and +compare the output against reference files. Many tests are a mixture +of these techniques. + +The `qtest `__ style of testing is to +test everything through the application. So effectively most testing +is "integration testing" or "end-to-end testing". + +For details about ``qtest``, consult the `QTest Manual +`__. As you read +it, keep in mind that, in spite of the recent date on the file, the +vast majority of that documentation is from before 2007 and predates +many test frameworks and approaches that are in use today. + +Notes on testing: + +- In most cases, things in the code are tested through integration + tests, though the test suite is very thorough. Many tests are driven + through the ``qpdf`` CLI. Others are driven through other files in + the ``qpdf`` directory, especially ``test_driver.cc`` and + ``qpdf-ctest.c``. These programs only use the public API. + +- In some cases, there are true "unit tests", but they are exercised + through various stand-alone programs that exercise the library in + particular ways, including some that have access to library + internals. These are in the ``libtests`` directory. + +Coverage +~~~~~~~~ + +You wil see calls to ``QTC::TC`` throughout the code. This is a +"manual coverage" system described in depth in the qtest documentation +linked above. It works by ensuring that ``QTC::TC`` is called sometime +during the test in each configured way. In brief: + +- ``QTC::TC`` takes two mandatory options and an optional one: + + - The first two arguments must be *string literals*. This is because + ``qtest`` finds coverage cases lexically. + + - The first argument is the scope name, usually ``qpdf``. This means + there is a ``qpdf.testcov`` file in the source directory. + + - The second argument is a case name. Each case name appears in + ``qpdf.testcov`` with a number after it, usually ``0``. + + - If the third argument is present, it is a number. ``qtest`` + ensures that the ``QTC::TC`` is called for that scope and case at + least once with the third argument set to every value from ``0`` + to ``n`` inclusive, where ``n`` is the number after the coverage + call. + +- ``QTC::TC`` does nothing unless certain environment variables are + set. Therefore, ``QTC:TC`` calls should have no side effects. (In + some languages, they may be disabled at compile-time, though qpdf + does not actually do this.) + +So, for example, if you have this code: + +.. code-block:: C++ + + QTC::TC("qpdf", "QPDF eof skipping spaces before xref", + skipped_space ? 0 : 1); + + +and this line in `qpdf.testcov`: + +:: + + QPDF eof skipping spaces before xref 1 + +the test suite will only pass if that line of code was called at least +once with ``skipped_space == 0`` and at least once with ``skipped_space +== 1``. + +The manual coverage approach ensures the reader that certain +conditions were covered in testing. Use of ``QTC::TC`` is only part of +the overall strategy. + +I do not require testing on pull requests, but they are appreciated, +and I will not merge any code that is not tested. Often someone will +submit a pull request that is not adequately tested but is a good +contribution. In those cases, I will often take the code, add it with +tests, and accept the changes that way rather than merging the pull +request as submitted. + +Personal Comments +----------------- + +QPDF started as a work project in 2002. The first open source release +was in 2008. While there have been a handful of contributors, the vast +majority of the code was written by one person over many years as a +side project. + +I maintain a very strong commitment to backward compatibility. As +such, there are many aspects of the code that are showing their age. +While I believe the codebase to have high quality, there are things +that I would do differently if I were doing them from scratch today. +Sometimes people will suggest changes that I like but can't accept for +backward compatibility reasons. + +While I welcome contributions and am eager to collaborate with +contributors, I have a high bar. I only accept things I'm willing to +maintain over the long haul, and I am happy to help people get +submissions into that state. diff --git a/manual/index.rst b/manual/index.rst index 8fcb6fa0..7b24fd3f 100644 --- a/manual/index.rst +++ b/manual/index.rst @@ -30,6 +30,7 @@ documentation, please visit `https://qpdf.readthedocs.io library weak-crypto json + contributing design qpdf-job linearization