Add documentation for code formatting and contribution guidelines

Jay Berkenbilt
1 parent ece6b6fe
Showing 6 changed files with 208 additions and 55 deletions
README-maintainer
TODO
libqpdf/sph/sph_types.h.new
manual/CMakeLists.txt
manual/contributing.rst
manual/index.rst
@@ -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.
@@ -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
 =====
@@ -40,6 +40,7 @@ SET(MANUAL_DEPS
   index.rst
   installation.rst
   json.rst
+  contributing.rst
   library.rst
   license.rst
   linearization.rst
+.. _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 <https://qtest.sourceforge.io>`__ 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
+<https://qtest.sourceforge.io/doc/qtest-manual.html>`__. 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.
@@ -30,6 +30,7 @@ documentation, please visit `https://qpdf.readthedocs.io
    library
    weak-crypto
    json
+   contributing
    design
    qpdf-job
    linearization