From 08c913f0352e0527c7a792a7bdb6d3cb517e19ca Mon Sep 17 00:00:00 2001 From: Charles Ofria Date: Thu, 23 Sep 2021 11:27:56 -0400 Subject: [PATCH] Update to various code guide lines with extensive additions for rules on commenting code. --- doc/dev/contribution-guidelines-and-review.md | 95 +++++++++++++++++-- 1 file changed, 87 insertions(+), 8 deletions(-) diff --git a/doc/dev/contribution-guidelines-and-review.md b/doc/dev/contribution-guidelines-and-review.md index 374b28474a..7d5c9223a5 100644 --- a/doc/dev/contribution-guidelines-and-review.md +++ b/doc/dev/contribution-guidelines-and-review.md @@ -46,12 +46,9 @@ guidelines below are for consistency. ## Guidelines based on Emscripten Limitations -> - Try to avoid use of 64-bit integers (that is, the "long long" -> type). Emscripten has to emulate these and they can cause a -> notable slowdown. > - Do not rely on exceptions when possible. Emscripten is slow at > dealing with them and they can slow down code even when not -> triggered. +> triggered. By default, we compile with exceptions disabled. > - Do not write multi-threaded code that uses shared state. > Javascript cannot (yet) handle such code and as such Emscripten > cannot compile it. Note that Emscripten does have experimental @@ -64,13 +61,85 @@ Please see the [Emscripten doc page](https://kripken.github.io/emscripten-site/docs/porting/guidelines/portability_guidelines.html) for a full list. +## Commenting in files + +All code should be well-commented such that it can be understood by a skilled +C++ programmer that is not familiar with the code base. Comments should be +written in Doxygen format where appropriate. + +Each file should have a header at the top to describe the goals of that file. +This header should include copyright information as well as the name of the file, +a brief description, and its status. Statuses include: + +| Status | Meaning +| ------ | ------- +| DESIGN | Notes are in place, and some functionality may work, but needs more engineering. +| ALPHA | Some basic functionality works, but more features still need to be added and tested +| BETA | Basic functionality is all in place, but needs more thorough testing. +| RELEASE | Well-tested functionality and used in multiple projects, at least by authors +| STABLE | Used by many non-authors in assorted projects without fixes for extended period +| BROKEN | Once worked (at least BETA level), but now needs to be repaired (not abandoned!) +| DEPRECATED | Functionality has been replaced and should shift to replacement. + +An example header might look like: + +``` +/** + * @note This file is part of Empirical, https://github.com/devosoft/Empirical + * @copyright Copyright (C) Michigan State University, MIT Software license; see doc/LICENSE.md + * @date 2016-2020. + * + * @file Ptr.hpp + * @brief A wrapper for pointers that does careful memory tracking (but only in debug mode). + * @note Status: BETA + * + * Ptr objects behave as normal pointers under most conditions. However, if a program is + * compiled with EMP_TRACK_MEM set, then these pointers perform extra tests to ensure that + * they point to valid memory and that memory is freed before pointers are released. + * + * If you want to prevent pointers to pointers (a common source of errors, but MAY be done + * intentionally) you can define EMP_NO_PTR_TO_PTR + * + * If you trip an assert, you can re-do the run a track a specific pointer by defining + * EMP_ABORT_PTR_NEW or EMP_ABORT_PTR_DELETE to the ID of the pointer in question. This will + * allow you to track the pointer more easily in a debugger. + * + * @todo Track information about emp::vector and emp::array objects to make sure we don't + * point directly into them? (A resize() could make such pointers invalid!) Or better, warn + * it vector memory could have moved. + * @todo Get working with threads + */ + ``` + +Each class should have at least a one-sentence description of the goals of that +class (unless it is the ONLY class in a file and the descriptions would be identical). +Each function should have at least a one-sentence description; parameters and +return value should also be described unless obvious from the function/parameter names. + +Sections of code should have a comment at the top, explaining what this section does. +More detailed comments are only needed if that section might be non-intuitive for an +outside programmer. Detailed comment can be either in the form of a fuller explanation +at the top of a section, or line-by-line hand holding. + +Comments should always focus on intention and reasoning, not merely restating +what the code is obviously doing. +Comments are especially criticial for bug fixes or for warnings in non-intuitive +code -- it is important to not just indicate what is now happening, but why +seemingly intuitive alternative methods are not correct, especially if it seems like it +would be simpler code. +Make sure to always FIX comments when you change code -- out of date comments +are far worse than no comments at all. + + ## General Standards All plain-text files should have line widths of 100 characters or less -unless that is unsupported for the particular file format. +unless that is unsupported for the particular file format or creates a +major loss in readability. All contributions should have their spelling checked before being -committed to the codebase. +committed to the codebase. For example, the VSCode plug-in +"Code Spell Checker" is a good choice. Vim users can run: @@ -80,6 +149,16 @@ Vim users can run: to automagically check the spelling within the file being edited. +If there is an Empirical replacement for a standard C++ feature in +the include/emp/base/ directory, you should use that replacement. +Specific cases include: +> - Use emp::Ptr<> instead of raw pointers. +> - Use emp_assert() instead of standard library assert(). +> - Use emp::array istead of std::array. +> - Use emp::optional instead of std::optional. +> - Use emp::vector instead of std::vector. + + ## Checklist Copy and paste the following into a pull request comment when it is @@ -87,8 +166,8 @@ ready for review: - [ ] Is it mergeable? - [ ] Did it pass the tests? - - [ ] Does 'make doc' succeed? - - [ ] If you added code, is it tested? Look at the output for 'make diff-cover' + - [ ] Does the source code follow the Empirical coding standards? + - [ ] Has the code been commented (especially non-intuitive sections) - [ ] Was a spellchecker run on the source code and documentation after changes were made?