Merge pull request #49 from lanl/jmm/formatting-automation

Add automation to formatting and extend development docs

.clang-format

@@ -1,4 +1,4 @@
-ColumnLimit: 80
+ColumnLimit: 90
 IndentWidth: 2
 AccessModifierOffset: -1
 
@@ -7,4 +7,4 @@ AllowShortIfStatementsOnASingleLine: true
 AlwaysBreakTemplateDeclarations: true
 BinPackArguments: true
 BinPackParameters: true
-ConstructorInitializerAllOnOneLineOrOnePerLine: false
+ConstructorInitializerAllOnOneLineOrOnePerLine: false

CMakeLists.txt

@@ -35,6 +35,7 @@ option (PORTS_OF_CALL_BUILD_TESTING "Test the current installation")
 
 # CONFIGURATION LOGIC
 # ----------------------------------------
+set(CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/cmake)
 
 # TARGET CONFIGURATION
 # ----------------------------------------
@@ -72,6 +73,10 @@ if(PORTS_OF_CALL_BUILD_TESTING)
  add_subdirectory(test)
 endif()
 
+# FORMATTING
+# ----------------------------------------
+include(Format)
+
 # INSTALL & EXPORT
 # ----------------------------------------
 

cmake/Format.cmake

@@ -0,0 +1,66 @@
+#=========================================================================================
+# (C) (or copyright) 2024. Triad National Security, LLC. All rights reserved.
+#
+# This program was produced under U.S. Government contract 89233218CNA000001 for Los
+# Alamos National Laboratory (LANL), which is operated by Triad National Security, LLC
+# for the U.S. Department of Energy/National Nuclear Security Administration. All rights
+# in the program are reserved by Triad National Security, LLC, and the U.S. Department
+# of Energy/National Nuclear Security Administration. The Government is granted for
+# itself and others acting on its behalf a nonexclusive, paid-up, irrevocable worldwide
+# license in this material to reproduce, prepare derivative works, distribute copies to
+# the public, perform publicly and display publicly, and to permit others to do so.
+#=========================================================================================
+
+find_program(
+ CLANG_FORMAT
+ NAMES
+ clang-format-12 # Debian package manager, among others, provide this name
+ clang-format-mp-12.0 # MacPorts
+ clang-format # Default name
+ )
+
+if (CLANG_FORMAT AND NOT CLANG_FORMAT_VERSION)
+ # Get clang-format --version
+ execute_process(
+ COMMAND ${CLANG_FORMAT} --version
+ OUTPUT_VARIABLE CLANG_FORMAT_VERSION_OUTPUT)
+
+ if (CLANG_FORMAT_VERSION_OUTPUT MATCHES "clang-format version ([0-9]+\.[0-9]+\.[0-9]+)")
+ set(CLANG_FORMAT_VERSION ${CMAKE_MATCH_1})
+ message(STATUS "clang-format --version: " ${CLANG_FORMAT_VERSION})
+
+ set(CLANG_FORMAT_VERSION ${CLANG_FORMAT_VERSION} CACHE STRING "clang-format version")
+ endif()
+endif()
+
+if (NOT CLANG_FORMAT_VERSION)
+ message(
+ WARNING
+ "Couldn't determine clang-format version. clang-format 12.0 is \
+ expected - results on other versions may not be stable")
+
+ set(CLANG_FORMAT_VERSION "0.0.0" CACHE STRING "clang-format version not found")
+elseif (NOT (CLANG_FORMAT_VERSION VERSION_GREATER_EQUAL "12.0" AND
+ CLANG_FORMAT_VERSION VERSION_LESS "13.0"))
+ message(
+ WARNING
+ "clang-format version 12.0 is required - results on other \
+ versions may not be stable")
+endif()
+
+# Specifically trying to exclude external here - I'm not sure if there's a better way
+set(
+ GLOBS
+ ${PROJECT_SOURCE_DIR}/spiner/[^\.]*.cpp ${PROJECT_SOURCE_DIR}/spiner/[^\.]*.hpp
+ ${PROJECT_SOURCE_DIR}/ports-of-call/[^\.]*.cpp ${PROJECT_SOURCE_DIR}/ports-of-call/[^\.]*.hpp
+ ${PROJECT_SOURCE_DIR}/test/[^\.]*.cpp ${PROJECT_SOURCE_DIR}/test/[^\.]*.hpp
+ ${PROJECT_SOURCE_DIR}/installtest/[^\.]*.cpp ${PROJECT_SOURCE_DIR}/installtest/[^\.]*.hpp
+)
+
+file(GLOB_RECURSE FORMAT_SOURCES CONFIGURE_DEPENDS ${GLOBS})
+
+if (CLANG_FORMAT)
+ add_custom_target(format_ports
+ COMMAND ${CLANG_FORMAT} -i ${FORMAT_SOURCES}
+ VERBATIM)
+endif()

doc/sphinx/src/developing.rst

@@ -8,3 +8,136 @@ required. We do require the files be formatted using
 ``clang-format``. The format version is pinned 12. Please call
 ```clang-format`` on your files before merging them.
 
+You may do so automatically either by making the ``format_ports``
+target in the build system, e.g.,
+
+.. code-block:: bash
+
+ mkdir -p ports-of-call/build
+ cd ports-of-call/build
+ cmake ..
+ make format_ports
+
+or you may call the ``format.sh`` script in the ``scripts``
+directory. This script supports two environment variables, ``CFM`` and
+``VERBOSE``. ``CFM`` is the path (or name) of your ``clang-format``
+executable. ``VERBOSE=1`` will cause it to output all files it
+modifies.
+
+The docs are built but not deployed on PRs from forks, and the
+internal tests will not be run automatically. So when the code is
+ready for merge, you must ask a project maintainer to trigger the
+remaining tests for you.
+
+Expectations for code review
+-----------------------------
+
+From the perspective of the contributor
+````````````````````````````````````````
+
+Code review is an integral part of the development process
+for ``ports-of-call``. You can expect at least one, perhaps many,
+core developers to read your code and offer suggestions.
+You should treat this much like scientific or academic peer review.
+You should listen to suggestions but also feel entitled to push back
+if you believe the suggestions or comments are incorrect or
+are requesting too much effort.
+
+Reviewers may offer conflicting advice, if this is the case, it's an
+opportunity to open a discussion and communally arrive at a good
+approach. You should feel empowered to argue for which of the
+conflicting solutions you prefer or to suggest a compromise. If you
+don't feel strongly, that's fine too, but it's best to say so to keep
+the lines of communication open.
+
+Big contributions may be difficult to review in one piece and you may
+be requested to split your pull request into two or more separate
+contributions. You may also receive many "nitpicky" comments about
+code style or structure. These comments help keep a broad codebase,
+with many contributors uniform in style and maintainable with
+consistent expectations accross the code base. While there is no
+formal style guide for now, the regular contributors have a sense for
+the broad style of the project. You should take these stylistic and
+"nitpicky" suggestions seriously, but you should also feel free to
+push back.
+
+As with any creative endeavor, we put a lot of ourselves into our
+code. It can be painful to receive criticism on your contribution and
+easy to take it personally. While you should resist the urge to take
+offense, it is also partly code reviewer's responsiblity to create a
+constructive environment, as discussed below.
+
+Expectations of code reviewers
+````````````````````````````````
+
+A good code review builds a contribution up, rather than tearing it
+down. Here are a few rules to keep code reviews constructive and
+congenial:
+
+* You should take the time needed to review a contribution and offer
+ meaningful advice. Unless a contribution is very small, limit
+ the times you simply click "approve" with a "looks good to me."
+
+* You should keep your comments constructive. For example, rather than
+ saying "this pattern is bad," try saying "at this point, you may
+ want to try this other pattern."
+
+* Avoid language that can be misconstrued, even if it's common
+ notation in the commnunity. For example, avoid phrases like "code
+ smell."
+
+* Explain why you make a suggestion. In addition to saying "try X
+ instead of Y" explain why you like pattern X more than pattern Y.
+
+* A contributor may push back on your suggestion. Be open to the
+ possibility that you're either asking too much or are incorrect in
+ this instance. Code review is an opportunity for everyone to learn.
+
+* Don't just highlight what you don't like. Also highlight the parts
+ of the pull request you do like and thank the contributor for their
+ effort.
+
+Some notes on style and code architecture
+``````````````````````````````````````````
+
+* A major influence on code style and architecture is the
+ `ten rules for developing safety-critical code`_, by Gerard Holzmann.
+ Safety critical code is code that exists in a context where failure
+ implies serious harm. A flight controler on an airplane or
+ spacecraft or the microcontroller in a car are examples of
+ safety-critical contexts. ``ports-of-call`` is not safety-critical
+ but many of the coding habits advocated for by Holzmann produce
+ long-lived, easy to understand, easy to parse, and easy to maintain code.
+ And we take many of the rules to heart. Here are a few that are most
+ relevant to ``ports-of-call``. They have been adapted slightly to
+ our context.
+
+ #. Avoid complex flow constructs such as gotos.
+
+ #. All loops must have fixed bounds. This prevents runaway
+ code. (Note this implies that as a general rule, one should use
+ ``for`` loops, not ``while`` loops. It also implies one should
+ keep recursion to a minimum.)
+
+ #. Heap memory allocation should only be performed at
+ initialization. Heap memory de-allocation should only be
+ performed at cleanup.
+
+ #. Restrict the length of functions to a single printed page.
+
+ #. Restrict the scope of data to the smallest possible.
+
+ #. Limit pointer use to a single dereference. Avoid pointers of
+ pointers when possible.
+
+ #. Be compiler warning aware. Try to address compiler warnings as
+ they come up.
+
+.. _ten rules for developing safety-critical code: http://web.eecs.umich.edu/~imarkov/10rules.pdf
+
+* ``ports-of-call`` is a modern C++ code
+ and both standard template library capabilities and template
+ metaprogramming are leveraged frequently. This can sometimes make
+ parsing the code difficult. If you see something you don't
+ understand, ask. It may be it can be refactored to be more simple or
+ better documented.