From d8078cfdd9f9002e234fc8bff88d3b57f9f42c9a Mon Sep 17 00:00:00 2001 From: Riley Murray Date: Mon, 17 Aug 2026 22:38:34 -0700 Subject: [PATCH] style and contributor guides --- CONTRIBUTING.md | 137 ++++++++++++++ README.md | 4 +- STYLE_AUDIT.md | 188 +++++++++--------- STYLE_GUIDE.md | 491 ++++++++++++------------------------------------ 4 files changed, 346 insertions(+), 474 deletions(-) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..7227679d --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,137 @@ +# Contributing to RandBLAS + +RandBLAS is a header-only C++ library with performance-sensitive kernels and +reproducible random-number generation. This guide explains the project +constraints and development workflow that matter when changing it. + +Follow the [`STYLE_GUIDE.md`](STYLE_GUIDE.md) for source and documentation +conventions. + +## Before you start + +Open an issue or talk with the maintainers on the +[RandLAPACK Discord server](https://discord.gg/R4qj8Er9YW) before making a +large public-API change or undertaking a performance optimization. A small +bug fix does not need a design discussion first. + +Read the documentation closest to your change: + +- The [FAQ](https://randblas.readthedocs.io/en/stable/FAQ.html), + [tutorial](https://randblas.readthedocs.io/en/stable/tutorial/index.html), + and [API reference](https://randblas.readthedocs.io/en/stable/api_reference/index.html) + describe the user-facing library. +- [`RandBLAS/DevNotes.md`](RandBLAS/DevNotes.md) describes the main + implementation, and + [`RandBLAS/sparse_data/DevNotes.md`](RandBLAS/sparse_data/DevNotes.md) + describes the sparse kernels and dispatchers. +- [`test/DevNotes.md`](test/DevNotes.md) and the other `DevNotes.md` files + describe specialized tests and implementation details. +- Pages 17--28 of the + [2024 LDRD final report](https://www.osti.gov/servlets/purl/2462906) provide + broader project background. + +Developer notes describe the implementation at the time they were written. +Check the code and tests when a note appears stale, and update the note if your +change makes it inaccurate. + +## Project constraints + +Preserve these properties unless a change explicitly sets out to revise one: + +- RandBLAS requires C++20 and exposes BLAS-like operations. +- Random generation goes through Random123 and RandBLAS's `RNGState` type. + Do not use `std::random` outside a test with a specific reason for it. +- A sampled random matrix must be identical for every OpenMP thread count. + Partition random-number streams by logical data position, not by thread + arrival order. +- Do not expose standard-library data structures in the public API. For this + rule, the public API is the set of declarations selected under + [`rtd/source/api_reference/`](rtd/source/api_reference/). +- Preserve memory ownership, thread safety, numerical behavior, and public API + compatibility unless the proposal and review call for a deliberate change. + +Keep patches focused. Do not mix a functional change with a repository-wide +formatting pass. + +## Set up a development environment + +For a standalone clone, follow [`INSTALL.md`](INSTALL.md). Its installer and +plain-CMake instructions are the supported starting points for Linux, macOS, +and Windows. + +The maintainers validate changes in the Spack-backed RandNLA workspace. +Homebrew may provide tools for a standalone macOS build, but it is not the +reference development environment. If you are using the RandNLA workspace and +its build directory is configured, run: + +```bash +cd /path/to/randnla/dev +source sourceme.sh +make -C build-randblas -j +ctest --test-dir build-randblas --output-on-failure +``` + +To build the examples after a library change, install the headers and rebuild +the separate examples tree: + +```bash +cd /path/to/randnla/dev +source sourceme.sh +make -C build-randblas install +make -C build-randblas-examples -j +``` + +## Test the change + +Put a regression test at the same abstraction level as the behavior: + +- random-number tests under `test/basic_rng/`; +- data-structure tests under `test/datastructures/`; +- linear-operation and sparse-dispatch tests under `test/linops/`. + +Run focused tests while developing. Before requesting review for a code +change, run the full suite for your configured build. If you use the RandNLA +workspace, use the Spack-backed commands shown above; maintainers use that +environment for final validation. + +Sampling changes need tests with more than one OpenMP thread count. Sparse +dispatch changes need coverage for every affected combination of storage +format, transpose flags, and dense layout. `left_spmm` has twelve principal +paths; `right_spmm` transforms its inputs and delegates to that dispatcher. + +## Update the documentation + +Update documentation in the same patch when behavior, a public declaration, +or an implementation rationale changes. + +- Public declarations need comments that render through Sphinx and Breathe. + The style guide explains the supported comment forms. +- API additions or removals may require a corresponding directive under + `rtd/source/api_reference/`. +- Tutorials, FAQ material, and other public prose belong under `rtd/source/`. +- Implementation rationale belongs in the nearest `DevNotes.md`. + +[`rtd/DevNotes.md`](rtd/DevNotes.md) describes the website build. Check the +rendered result when changing public comments or reStructuredText, rather than +judging only the Doxygen XML. + +## Measure performance changes + +Discuss a performance optimization with the maintainers before undertaking +it. Benchmark the old and new implementations under comparable conditions, +then report the matrix sizes, compiler, BLAS backend, OpenMP configuration, +thread count, and results. + +A faster result in one configuration does not establish a general +improvement. Keep correctness tests separate from benchmark evidence. + +## Before requesting review + +- [ ] The patch has one clear purpose and no unrelated formatting sweep. +- [ ] Public API names and argument order match neighboring BLAS-like operations. +- [ ] Ownership, RNG-state, and behavioral changes are documented. +- [ ] Focused tests pass; code changes also pass the full test suite. +- [ ] Random output is independent of the OpenMP thread count. +- [ ] Sparse changes cover every affected dispatch path. +- [ ] Public comments render through the Sphinx--Breathe pipeline. +- [ ] Performance claims include comparable before-and-after measurements. diff --git a/README.md b/README.md index 81df39b9..8efa7285 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,9 @@ We have three types of documentation. 2. Web documentation, split into a [tutorial](https://randblas.readthedocs.io/en/latest/tutorial/index.html) and an [API reference](https://randblas.readthedocs.io/en/latest/api_reference/index.html). 3. Developer notes; [one](RandBLAS/DevNotes.md) for RandBLAS as a whole and [another](RandBLAS/sparse_data/DevNotes.md) for our sparse matrix functionality. -Contributors should also consult the [RandBLAS style guide](STYLE_GUIDE.md). +See [`CONTRIBUTING.md`](CONTRIBUTING.md) before preparing a change. +Source and documentation conventions are in +[`STYLE_GUIDE.md`](STYLE_GUIDE.md). Detailed installation instructions are in [INSTALL.md](INSTALL.md). diff --git a/STYLE_AUDIT.md b/STYLE_AUDIT.md index 85c53c52..84e894f6 100644 --- a/STYLE_AUDIT.md +++ b/STYLE_AUDIT.md @@ -1,8 +1,10 @@ # RandBLAS style audit -This audit records the evidence behind `STYLE_GUIDE.md`. -The guide is normative; this file is descriptive. -Keeping the two separate prevents an old inconsistency from becoming a new rule. +This audit records the evidence behind `STYLE_GUIDE.md` and the +project-specific constraints summarized in `CONTRIBUTING.md`. +The guides are normative; this file is descriptive. +Keeping normative guidance separate from the audit prevents an old +inconsistency from becoming a new rule. ## Review snapshot @@ -40,8 +42,14 @@ We call a convention a **strong consensus** when it appears in at least five eligible files and at least 90% of eligible occurrences. A **working consensus** needs five eligible files and a share of at least 70%. Everything else is **mixed**. -Mixed evidence is either omitted from the guide, split by file stratum, or -presented as an explicit forward-looking recommendation. +Mixed evidence is omitted from the guide or left to the surrounding file. +The audit does not turn a close count into a new formatting rule. + +The review also uses the original February 2025 contributor notes, now +preserved in [issue #99](https://github.com/BallisticLA/RandBLAS/issues/99#issuecomment-5323627413). +Those notes state project requirements that occurrence counts cannot recover, +including the Sphinx–Breathe documentation format, the Random123 boundary, +and the definition of the public API. ## Corpus @@ -175,9 +183,7 @@ Braces around single-statement control bodies are mixed. The scan found 742 same-line braced controls and 285 candidates whose body starts on the following line without a same-line brace. The latter pattern appears in core headers, tests, and examples. -The guide requires braces when a body has multiple statements or nested -control flow; a single short statement may remain unbraced when the local -code is clear. +The guide therefore leaves body bracing to the surrounding file. ### Whitespace and line length @@ -197,10 +203,8 @@ The repository does not establish a clean trailing-whitespace convention: 453 lines in 44 files contain it. Nor does it support a hard 100-column limit: 514 lines in 43 files exceed that length, often for signatures, formulas, macro bodies, or URLs. -The guide treats both as forward-looking recommendations: remove trailing -whitespace and wrap prose or code when doing so helps a reader, but do not -distort a mathematical expression or tabular signature merely to hit a -number. +The guide therefore sets no rule for trailing whitespace and no numerical +line limit. ### Pointer and reference binding @@ -210,11 +214,10 @@ resembling `T *ptr`. The split changes by stratum: examples favor `T* ptr`, while recent sparse kernels and much of the public API favor `T *ptr`. -References are much less ambiguous. -The same scan found 282 occurrences resembling `T &ref` and 47 resembling -`T& ref`. -The guide recommends declarator binding (`T *ptr`, `T &ref`) for new code and -labels the pointer half as a recommendation rather than observed consensus. +References favor `T &ref`: the same scan found 282 such occurrences and 47 +resembling `T& ref`. +That does not justify one combined pointer-and-reference rule. +The guide leaves both choices to the declaration being modified. ### Naming and API shape @@ -238,9 +241,17 @@ The library uses BLAS++ vocabulary throughout: the scan found 127 `blas::Layout` references and 128 `blas::Op` references. It found 953 `int64_t` references and 174 `randblas_require(...)` calls in headers. -These counts support the existing API pattern: explicit layout/operation -flags, signed 64-bit dimensions and strides, constrained signed index types, -and validation near public entry points. +The type counts support explicit layout/operation flags, signed 64-bit +dimensions and strides, and constrained signed index types. +The validation calls show that checks are common, but a raw call count does +not establish that every public entry point must validate every property. +The guide therefore makes no blanket validation rule. + +The February 2025 notes provide two API constraints directly. +`std::random` is prohibited outside selected tests; all other randomness goes +through Random123 and `RNGState`. +Standard-library data structures do not appear anywhere in the public API, +which the notes define as the declarations selected by the web API reference. Concepts are used directly in template parameter lists when the compiler supports them. @@ -260,16 +271,8 @@ The library contains ten OpenMP pragmas. Indented preprocessor directives are common inside function bodies: the scan found 106 in library headers, 14 in tests, and 16 in examples. They are mostly short-lived matrix-index macros or compiler branches. -The pattern is deliberate enough to document, but these macros should be -undefined as soon as their local job is complete. - -Macro naming has two domains. -Public/compiler configuration names are uppercase or carry a recognizable -project prefix (`RandBLAS_HAS_OpenMP`, `RandBLAS_OPTIMIZE_OFF`). -Function-like validation macros keep their existing lower-case API spelling -(`randblas_require`, `randblas_error_if`). -The lower-case `matA` macros in `RandBLAS/util.hh` are legacy local names, not -a convention for new macros. +Their spelling and placement vary too much to support an additional guide +rule. ### Source documentation @@ -277,25 +280,40 @@ Nineteen of 32 headers contain `///` documentation; six contain at least one `/** ... */` block. Across C++ files, the scan found 3,225 `///` lines and 17 block-comment openers. -Only two headers use `@file`, so that tag is optional rather than a file -template requirement. - -All 77 detected parameter direction annotations use `@param[in]`. -The public docs also use `@tparam`, `@returns`, and project math commands such -as `\math{...}`. -This establishes `///` plus explicit Doxygen fields as the preferred form for -public contracts. -Implementation comments are most useful when they explain an invariant, -dispatch choice, numerical concern, or performance tradeoff. +This supports `///` for web-facing declarations. + +The original audit made a bad inference from Doxygen field counts. +Native `@param`, `@tparam`, and return fields occur in only four headers; +60 of the parameter fields are concentrated in `RandBLAS/skge.hh`'s lower +level kernels. +By contrast, six public-operation headers contain 220 parameter rows in the +embedded reStructuredText form `name - [direction]`. +The February 2025 notes explicitly reject native Doxygen fields for new +web-facing comments because of their rendered appearance. + +The build configuration confirms why. +`rtd/source/conf.py` runs Doxygen to produce XML, configures Breathe to read +that XML, and then lets Sphinx render the site. +The API-reference pages select declarations with Breathe directives such as +`doxygenfunction` and `doxygenstruct`. +The C++ comments use `@verbatim embed:rst:leading-slashes`, the `\math{...}` +Doxygen alias, reStructuredText math roles, and the custom `mathmacro` +directive to control the Sphinx output. +Doxygen is an extraction stage, not the documentation frontend. + +The library headers contain 81 major equals-sign separators and 113 hyphen +separators. +Library, test, and example code contain 48 `// MARK:` labels. +Together with the contributor notes, these counts support the short +navigational guidance in the revised guide. Web documentation is written in reStructuredText. The current source has 62 underlined headings, 272 inline `:math:` roles, and six note/warning directives. Repository notes are Markdown and contain 90 fenced-code markers and 121 ATX headings. -The two formats have distinct jobs: public tutorials/API pages belong under -`rtd/source/`, while implementation rationale belongs in the nearest -`DevNotes.md`. +Public tutorials and API pages belong under `rtd/source/`; implementation +rationale belongs in the nearest `DevNotes.md`. ### Tests @@ -304,11 +322,6 @@ and four plain `TEST` definitions. Fixture classes normally begin with `Test`; test names are normally snake_case descriptions of behavior or parameter combinations. -The scan found 141 `EXPECT_*` and 76 `ASSERT_*` uses. -The counts do not imply that one family is preferred everywhere. -Use `ASSERT_*` when later statements cannot run safely after failure; use -`EXPECT_*` when independent checks can still provide useful information. - Tests are organized by abstraction level, not by source filename alone: basic RNG behavior, data structures, low-level linear operations, wrapper APIs, and testing helpers live in separate subdirectories. @@ -322,22 +335,14 @@ Of 400 detected command invocations, 398 use lower-case command names. Four-space continuation/block indentation appears on 224 lines, compared with 18 two-space lines. Project-facing variables retain their established spelling -(`RandBLAS_HAS_OpenMP`, `BUILD_TESTS`); new private helpers use a leading -`_rb_` prefix, as in `CMake/rb_summary.cmake`. - -Only three Python files are tracked. -They use four-space indentation but mix quote, import, and whitespace styles; -eight Python lines contain a tab or trailing whitespace. -The JavaScript and CSS strata each contain one file. -The guide therefore gives small-language recommendations rather than claims -of statistical consensus: use four spaces in Python, preserve the local -language's conventional formatter shape, and follow the surrounding file in -single-file strata. - -Shell, PowerShell, YAML, and workflow files are similarly task-specific. -They should favor explicit commands and platform terminology already used by -the installation and CI files. -No cross-language indentation rule is inferred from them. +(`RandBLAS_HAS_OpenMP`, `BUILD_TESTS`). +The `_rb_` private-helper spelling comes from one file and does not establish +a repository-wide rule. + +Only three Python files are tracked, while the JavaScript and CSS strata each +contain one file. +Those files do not support repository-wide rules beyond following the local +file, so the guide does not restate generic language advice for them. ## Outlier analysis @@ -401,8 +406,8 @@ work and serve local application code. Long lines, trailing whitespace, and duplicate includes are common enough in the seven-file example stratum that they do not distinguish these files as outliers. -The guide still recommends avoiding duplicate includes and cleaning trailing -whitespace in new edits. +Duplicate includes remain a mechanical defect; mixed line length and trailing +whitespace do not become guide rules. ### Result @@ -422,52 +427,35 @@ state. ## Guide traceability -The following table traces the guide's normative rules to repository evidence. -“Project requirement” means the rule comes from `AGENTS.md` or a DevNotes file -rather than a mechanical style count. -Recommendations are directions for new work, not claims that old files already -agree. +The following table traces the guides' normative rules to repository evidence. +“Project requirement” means the rule comes from `AGENTS.md`, a DevNotes file, +or the February 2025 contributor notes rather than a mechanical style count. -| Guide section | Rule set | Evidence | Confidence | Exceptions or limits | +| Document and section | Rule set | Evidence | Confidence | Exceptions or limits | |---|---|---|---|---| -| 2 | Preserve correctness, ownership, thread safety, and public API compatibility during style changes | `AGENTS.md`; `RandBLAS/DevNotes.md`; [close-reading method](#close-reading-anchors) | Project requirement | None; this is the safety boundary for cleanup work | -| 2 | Keep sampled matrices independent of thread count | `AGENTS.md`; `RandBLAS/DevNotes.md`; `test/datastructures/test_denseskop.cc` | Project requirement | None | -| 2 | Follow neighboring BLAS-like parameter order | [naming and API profile](#naming-and-api-shape) | Working consensus | Match the nearest operation because the signatures serve different kernels | -| 2, 9 | Benchmark kernel optimizations and record the relevant configuration | `AGENTS.md`; `RandBLAS/sparse_data/DevNotes.md` | Project requirement | Applies to performance changes, not formatting-only work | -| 2, 8 | Use comments for contracts, invariants, dispatch, numerical concerns, and performance rationale | [source-documentation profile](#source-documentation); `RandBLAS/DevNotes.md` | Working consensus | Short local comments may still label data or a compact transformation | -| 3 | Put the license first, use one `#pragma once`, and group project, third-party, then standard includes | [C++ structure and spacing](#c-structure-and-spacing); [include profile](#includes-and-preprocessor-code) | Strong consensus for license/guard/include syntax; working consensus for group order | `RandBLAS.hh` keeps its include guard and installed-path angle includes | -| 4 | Use four spaces, avoid tabs, add spaces to `template <...>` and inheritance, and place opening braces on the same line | [C++ structure and spacing](#c-structure-and-spacing); [whitespace profile](#whitespace-and-line-length) | Strong consensus, except template spacing in tests is working consensus | Existing test/type brace placement is mixed; use the rule for new code | -| 4 | Brace multi-statement or nested bodies; allow a clear, short single statement to follow local form | [C++ structure and spacing](#c-structure-and-spacing) | Correctness rule plus mixed local evidence | The guide deliberately does not require all single statements to be braced | -| 4 | Bind pointers and references to the declarator and remove trailing whitespace on touched lines | [pointer/reference profile](#pointer-and-reference-binding); [whitespace profile](#whitespace-and-line-length) | Recommendation for pointers and trailing whitespace; working consensus for references | Do not reformat untouched code solely to apply these recommendations | -| 5 | Use the documented naming table; keep local macros uppercase, narrow, and explicitly undefined | [naming profile](#naming-and-api-shape); [preprocessor profile](#includes-and-preprocessor-code) | Strong or working consensus by entity | Named public lower-case types and validation macros remain established exceptions | -| 6 | Reuse C++20 concepts and BLAS++ enum types; use `int64_t` dimensions/strides and validate public inputs | [naming and API profile](#naming-and-api-shape); `AGENTS.md` | Project requirement backed by strong occurrence counts | Sparse index buffers may use a constrained signed index type | -| 6 | Format long signatures one logical parameter per line and preserve documented ownership flags | [close-reading anchors](#close-reading-anchors); `RandBLAS/sparse_data/DevNotes.md` | Working convention for signatures; project requirement for ownership | Compact signatures may stay on one line when they remain readable | -| 7 | Make component headers self-contained; include direct dependencies; keep the OpenMP include conditional | [include profile](#includes-and-preprocessor-code); build model in `AGENTS.md` | Build requirement for direct dependencies; strong local convention for OpenMP | The installed umbrella header follows its documented exception | -| 8 | Prefer `///` public contracts with Doxygen fields and keep implementation rationale in DevNotes | [source-documentation profile](#source-documentation) | Strong consensus for `///`; working convention for field coverage | `@file` is optional; `@param[out]` extends the repository's direction syntax to output buffers | -| 9 | Put OpenMP pragmas next to their loops and cover every affected sparse format/transpose/layout path | [preprocessor profile](#includes-and-preprocessor-code); `AGENTS.md`; `RandBLAS/sparse_data/DevNotes.md`; `test/DevNotes.md` | Working convention for pragma placement; project requirement for path coverage | Coverage is limited to paths affected by the change | -| 10 | Use fixtures for shared setup, choose `ASSERT_*` for unsafe continuation, and use `EXPECT_*` for independent checks | [test profile](#tests) | Strong fixture convention; semantic GoogleTest rule | A plain `TEST` remains suitable when no shared setup exists | -| 10 | Give RNG changes deterministic checks and test new sketch operations across their applicable sides, transposes, layouts, submatrices, and formats | `AGENTS.md`; `test/DevNotes.md`; [test profile](#tests) | Project requirement | Only applicable combinations need coverage | -| 11 | Keep examples focused on a use or benchmark and keep local helper macros narrow | [example anchors](#close-reading-anchors); [rejected example seeds](#rejected-example-seeds) | Working editorial rule | Example-local timing and index macros are permitted | -| 11 | Use lower-case CMake commands, four-space blocks, established public variable spellings, and `_rb_` for private helpers | [CMake profile](#cmake-python-and-automation) | Strong for command case; working for indentation and helper names | Existing public/cache names retain their spelling | -| 11 | Use four spaces in Python; otherwise follow local form in small language strata and prefer explicit automation | [CMake, Python, and automation](#cmake-python-and-automation) | Recommendation | No repository-wide quote rule is inferred from three Python files | -| 12 | Preserve the two documented exceptions and do not copy their special forms into ordinary component headers or tests | [outlier register](#ranked-register) | Explicit exception | Applies only to `RandBLAS.hh` and upstream-shaped regions of `test_r123.cc` | +| `CONTRIBUTING.md`: Project constraints | Preserve correctness, ownership, thread safety, public API compatibility, and reproducibility | `AGENTS.md`; the three DevNotes files; issue #99 | Project requirement | A proposal may deliberately revise one of these properties | +| `CONTRIBUTING.md`: Project constraints | Route randomness through Random123/`RNGState`; keep standard-library data structures out of the web-defined public API | [issue #99 contributor notes](https://github.com/BallisticLA/RandBLAS/issues/99#issuecomment-5323627413) | Project requirement | Selected tests may use another generator for a stated reason | +| `CONTRIBUTING.md`: Development and review | Use the Spack-backed workspace for maintainer validation, test affected sparse dispatch paths, and benchmark optimizations | Workspace `AGENTS.md`; DevNotes; relevant tests | Project requirement | Standalone contributors use `INSTALL.md`; coverage is limited to paths affected by the change | +| `STYLE_GUIDE.md`: C++ source | Use four spaces, same-line braces, `template <...>` spacing, and the naming table | [C++ structure and spacing](#c-structure-and-spacing); [naming profile](#naming-and-api-shape) | Strong or working consensus by form | Pointer/reference spacing and line length remain local choices | +| `STYLE_GUIDE.md`: C++ source | Put the license first, use one `#pragma once`, include direct dependencies, and group includes | [C++ structure and spacing](#c-structure-and-spacing); [include profile](#includes-and-preprocessor-code) | Strong for protection and include syntax; working for group order | `RandBLAS.hh` keeps its installed-header form | +| `STYLE_GUIDE.md`: Public interfaces | Reuse C++20 concepts and BLAS++ enums; use `int64_t` for dimensions and strides | CMake configuration; [naming and API profile](#naming-and-api-shape) | Build requirement backed by occurrence counts | Sparse indices may use a constrained signed type | +| `STYLE_GUIDE.md`: Documentation | Treat Doxygen as XML extraction and write web-facing comments for Sphinx–Breathe | [source-documentation profile](#source-documentation); `rtd/source/conf.py`; `rtd/DevNotes.md`; issue #99 | Project requirement backed by build configuration | Native Doxygen fields remain in legacy comments | +| `STYLE_GUIDE.md`: Documentation | Use `///`, embedded reStructuredText for long parameter lists, project math facilities, and the established separators/markers | [source-documentation profile](#source-documentation); issue #99 | Strong convention plus project requirement | Short contracts should remain plain prose | +| `STYLE_GUIDE.md`: Other files | Use the established test fixture/case naming, lower-case CMake commands, and otherwise follow the local file | [test profile](#tests); [CMake, Python, and automation profile](#cmake-python-and-automation) | Strong test and CMake consensus; smaller strata are mixed | A plain `TEST` is suitable without shared setup | ## Rubric dry run -We applied the guide to five pairs without editing the source files. +We applied the guide to representative pairs without editing the source files. The dry run asks whether the guide distinguishes a strong rule from a -recommendation, whether deviations cluster, and whether provenance supplies a +local choice, whether deviations cluster, and whether provenance supplies a narrow exception. | Stratum and pair | Guide result | Outlier result | Ambiguity exposed | |---|---|---|---| -| Core: `RandBLAS/dense_skops.hh` / `RandBLAS/base.hh` | Both contain old local spacing and documentation forms. `base.hh` also has a duplicate `` and malformed `#include`, which violate two strong include rules. | `base.hh` is a focused cleanup candidate; `dense_skops.hh` is ordinary core code with localized debt. | A recommendation such as pointer binding cannot classify a file by itself. | +| Core: `RandBLAS/dense_skops.hh` / `RandBLAS/base.hh` | Both contain old local spacing and documentation forms. `base.hh` also has a duplicate `` and malformed `#include`, which violate two strong include rules. | `base.hh` is a focused cleanup candidate; `dense_skops.hh` is ordinary core code with localized debt. | Mixed pointer spacing and legacy Doxygen fields cannot classify a file by themselves. | | Sparse: `csr_spmm_impl.hh` / `csr_trsm_impl.hh` | The SpMM header follows the structural rules apart from minor include grouping. The TRSM header puts `#pragma once` before the license and repeats it after the license. | Only `csr_trsm_impl.hh` is a cleanup candidate. | None: the duplicate protection is a strong, mechanical defect. | | Tests: `test_lskge3.cc` / `test_denseskop.cc` | Both show older test-brace forms. `test_denseskop.cc` adds several unspaced templates and one malformed inheritance clause. | `test_denseskop.cc` has localized defects but is not an outlier; neither file has the independent cluster needed for that label. | Test brace placement is mixed, so the guide governs new code without retroactively condemning either file. | -| Examples: `tls_dense_skop.cc` / `svd_rank1_plus_noise.cc` | Both implement a recognizable numerical use and contain example-local rough edges. The SVD example's duplicate `` is a cleanup item, while its index macro is allowed when kept local. | Neither example is an outlier; the seeded SVD example scored 0.000 on eligible stratum rules. | “Teach a real use” is an editorial review question, not a mechanical outlier feature. | -| Other languages: `CMake/rb_config.cmake` / `rtd/source/conf.py` | The CMake file follows the reliable command-case and block-indentation rules. The Python file has mixed import and quote forms, but the guide makes those local-style questions. | Neither file is an outlier. | The small Python corpus supports an indentation recommendation, not a repository-wide formatter or quote rule. | The dry run produced no rule that reverses an evidence-based outlier classification. -It did expose two useful boundaries: recommendations cannot create an outlier -on their own, and mixed test/Python evidence must remain local guidance. +It exposed one useful boundary: mixed evidence must remain local guidance. diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md index 3c7f57f3..ef66f32d 100644 --- a/STYLE_GUIDE.md +++ b/STYLE_GUIDE.md @@ -1,435 +1,180 @@ # RandBLAS style guide RandBLAS is a header-only numerical library. -Its style exists to make templated, performance-sensitive code reviewable -without hiding the constraints that make the code correct. - -This guide applies to new and substantially modified first-party files. -It describes the repository at commit `952251c` and makes a few explicit -recommendations where the repository is mixed. -See [`STYLE_AUDIT.md`](STYLE_AUDIT.md) for counts, counterexamples, and the -outlier register. - -The words **must**, **should**, and **may** are deliberate. -“Must” marks a correctness or project requirement. -“Should” marks established house style. -“Recommendation” chooses a direction where the current files do not agree. - -## 1. Purpose and scope - -Use this guide for library headers, tests, examples, CMake, documentation, -and project automation. -Match the local file when a language has too little evidence for a -repository-wide rule. - -Existing deviations do not create a second style. -Do not combine a functional change with a broad cleanup merely because this -guide makes the cleanup easy to see. -The [outlier audit](STYLE_AUDIT.md#outlier-analysis) records good candidates -for later, focused work. - -## 2. Guiding principles - -1. **Correctness comes first.** Numerical behavior, memory ownership, - thread safety, and public API compatibility must survive a style change. -2. **Random generation is reproducible.** Sampling code must produce the - same random matrix regardless of the number of OpenMP threads. -3. **BLAS-like APIs stay recognizable.** Layout, operation, side, dimension, - stride, and scaling arguments should follow the order used by neighboring - RandBLAS entry points. -4. **Performance claims need measurements.** A kernel optimization must be - benchmarked before and after the change. -5. **Comments carry the reason.** Code states what happens; comments should - explain invariants, dispatch choices, numerical concerns, or performance - tradeoffs. -6. **Changes stay reviewable.** Prefer a focused patch over incidental - reformatting of an entire file. - -These principles come from `AGENTS.md`, `RandBLAS/DevNotes.md`, and the -sparse/testing DevNotes. - -## 3. File layout - -C++ source and component headers should use this order: - -1. the project license block; -2. one `#pragma once` for a header; -3. the file-level Doxygen comment, when useful; -4. RandBLAS includes; -5. third-party includes; -6. standard-library includes; -7. declarations and definitions inside the narrowest useful namespace. - -The library-header evidence is summarized under -[C++ structure and spacing](STYLE_AUDIT.md#c-structure-and-spacing). +This guide records the conventions for writing its source and documentation. +It applies to new and substantially modified code; it is not a reason for +drive-by reformatting. -```cpp -// Copyright, 2024. See LICENSE for copyright holder information. -// ...the complete project license block... - -#pragma once - -#include "RandBLAS/base.hh" -#include "RandBLAS/exceptions.hh" - -#include - -#include -#include -``` +See [`CONTRIBUTING.md`](CONTRIBUTING.md) for project constraints, development +setup, tests, and review preparation. +See [`STYLE_AUDIT.md`](STYLE_AUDIT.md) for the repository evidence behind the +guide. +When this guide is silent, follow the surrounding file. -Do not duplicate an include or write the directive without a separating -space: +## C++ source -```cpp -// Avoid. -#include -#include -``` - -`RandBLAS.hh` is an intentional exception. -It is the installed umbrella header, retains its include guard, and includes -installed `RandBLAS/` paths with angle brackets. -New component headers should use one `#pragma once`. +### Formatting -## 4. C++ formatting - -Indent blocks with four spaces and do not introduce tabs. -Put an opening brace on the same line as a namespace, type, function, test -macro, or control statement. -The repository follows this form in 92.3% of measured brace occurrences. +Use four spaces for indentation and do not introduce tabs. +Put opening braces on the same line as functions, types, namespaces, tests, +and control statements. +Write a space in `template <...>` and around an inheritance colon. ```cpp -namespace RandBLAS::sparse_data { - template void scale(int64_t n, T alpha, T *x) { for (int64_t i = 0; i < n; ++i) { x[i] *= alpha; } } - -} // namespace RandBLAS::sparse_data ``` -Avoid the following form in new code: - -```cpp -template -void scale(int64_t n, T alpha, T *x) -{ - // A tab was used for this indentation in the form being avoided. - for (int64_t i = 0; i < n; ++i) - x[i] *= alpha; -} -``` +The repository does not have a settled rule for spacing around `*` and `&`. +Follow the declaration you are modifying. -Use a space in `template <...>` and around an inheritance colon: +There is no fixed line-length limit. +Wrap prose, signatures, and expressions when doing so makes them easier to +read; do not damage a formula or compact tabular code to hit a column count. +For a long function signature, put one logical parameter on each line and the +closing parenthesis on its own line. -```cpp -template -class TestSparse : public ::testing::Test { -}; -``` +### Headers and includes -Braces must enclose a body with multiple statements or nested control flow. -A short single-statement body may remain unbraced when it is clear and agrees -with the surrounding code. +Put the project license first in a new header, followed by one `#pragma once`. +Include the headers a file uses rather than relying on transitive includes. +Group RandBLAS, third-party, and standard-library headers in that order. -Recommendation: bind `*` and `&` to the declarator in new code. -Pointer binding is mixed, while `T &ref` has a clear majority. +Use repository-qualified quotes for RandBLAS component headers and angle +brackets for external headers: ```cpp -void apply( - int64_t n, - const T *input, - T *output, - const RNGState<> &state -) { - // ... -} -``` +#include "RandBLAS/base.hh" + +#include -Recommendation: remove trailing whitespace on touched lines. -There is no hard line-length limit. -Wrap prose, signatures, and expressions when the result is easier to read; -do not damage a formula, URL, or compact tabular signature to satisfy an -arbitrary column count. -The mixed evidence is recorded under -[Whitespace and line length](STYLE_AUDIT.md#whitespace-and-line-length). +#include +``` -## 5. Naming +`RandBLAS.hh` is the installed umbrella header. +It intentionally keeps an include guard and uses installed `RandBLAS/` paths +in angle brackets. -Use the following names for new interfaces: +### Naming | Entity | Form | Examples | |---|---|---| -| Top-level namespace | `RandBLAS` | `RandBLAS` | -| Nested namespace | lower-case or snake_case | `dense`, `sparse_data` | -| Public type or concept | PascalCase | `DenseSkOp`, `CSRMatrix`, `SparseMatrix` | +| Namespace | `RandBLAS`; lower-case nested names | `RandBLAS::sparse_data` | +| Public type or concept | PascalCase | `DenseSkOp`, `SparseMatrix` | | Function | snake_case | `sketch_general`, `left_spmm` | | Variable or field | snake_case | `n_rows`, `next_state` | | Principal template type | short semantic uppercase | `T`, `RNG`, `SKOP` | -| Index/state alias | lower-case with `_t` | `sint_t`, `state_t` | -| Configuration/compiler macro | uppercase with a project prefix when public | `RandBLAS_HAS_OpenMP` | -| Test fixture | PascalCase beginning with `Test` | `TestDenseMoments` | -| Test name | snake_case behavior or case | `submatrix_a_double_colmajor` | - -The evidence and established exceptions are listed under -[Naming and API shape](STYLE_AUDIT.md#naming-and-api-shape). - -Keep the established BLAS-like abbreviations in APIs such as `lskge3`, -`rskges`, and `left_spmm`. -When a name's “left” or “right” is easy to misread, document which operand -occupies that position. +| Index or state alias | lower-case with `_t` | `sint_t`, `state_t` | -Local matrix-index macros should be uppercase. -Public validation macros retain their existing lower-case spellings. -A local macro must not escape the region that needs it; `#undef` it after its -last use. +Keep established BLAS-like abbreviations such as `lskge3`, `rskges`, and +`left_spmm`. +When “left” or “right” is easy to misread, document which operand the name +describes. -## 6. Public APIs and templates +### Public interface conventions RandBLAS requires C++20. -Templates should express existing constraints with `SignedInteger`, -`SketchingOperator`, `SketchingDistribution`, or `SparseMatrix` rather than -new SFINAE machinery. -Keep the compatibility branches that define those names as `typename` when -the relevant concept feature test is absent. - -Public dense and sparse operations should use BLAS++ types such as -`blas::Layout`, `blas::Op`, `blas::Side`, `blas::Uplo`, and `blas::Diag`. -Dimensions and strides should use `int64_t`; sparse index buffers may use a -`SignedInteger` template parameter. +Reuse its `SignedInteger`, `SketchingOperator`, `SketchingDistribution`, and +`SparseMatrix` concepts rather than adding parallel constraint machinery. +Use BLAS++ types such as `blas::Layout`, `blas::Op`, `blas::Side`, +`blas::Uplo`, and `blas::Diag` in BLAS-like APIs. +Dimensions and strides use `int64_t`; sparse index buffers may use a +`SignedInteger` template parameter. Match the parameter order of the nearest public operation. -A long signature should put one logical parameter on each line and keep the -closing parenthesis aligned with the declaration: - -```cpp -template -void left_spmm( - blas::Layout layout, - blas::Op opA, - blas::Op opB, - int64_t m, - int64_t n, - int64_t k, - T alpha, - const SpMat &A, - const T *B, - int64_t ldb, - T beta, - T *C, - int64_t ldc -) { - // ... -} -``` - -Public entry points must validate dimensions, strides, offsets, enum values, -and structural assumptions before a kernel relies on them. -Use the existing `randblas_require` and error helpers. - -Ownership must be visible in a type's contract. -Sparse matrix views and owning matrices share representations, so constructors -and destructors must preserve the documented ownership flags. -## 7. Includes and dependencies - -Every component header should compile when included on its own with the -project's configured dependencies. -Include what the file uses rather than depending on incidental transitive -includes. - -Within a component header, use repository-qualified quotes for project files -and angle brackets for BLAS++, Random123, OpenMP, and standard headers: - -```cpp -#include "RandBLAS/base.hh" -#include "RandBLAS/sparse_data/csr_matrix.hh" +Make ownership explicit in the contract of an owning object or view. -#include +## Documentation -#include -#include -``` +RandBLAS's website is built with Sphinx and Breathe; it is not a Doxygen +website. +The build has three stages: -Keep OpenMP's header conditional: +1. Doxygen parses the C++ declarations and writes XML. +2. Breathe reads that XML for directives such as `doxygenfunction` in the + API-reference pages. +3. Sphinx renders the reStructuredText pages and the reStructuredText embedded + in C++ comments. -```cpp -#if defined(RandBLAS_HAS_OpenMP) -#include -#endif -``` +Doxygen is therefore an extraction layer. +Write public documentation for the Sphinx output that readers actually see. -Do not add a new BLAS++ dependency casually. -The project currently relies on a small subset of BLAS operations while using -its enums broadly. -Discuss an expansion before it becomes part of the public dependency surface. +### Public C++ comments -## 8. Comments and documentation +Use `///` comments for declarations included in the web API reference. +For a function with a few arguments, explain the contract in plain prose. +Do not add a field for every parameter merely because Doxygen supports one. -Public contracts should use `///` Doxygen comments. -Document template parameters, input/output direction, dimensional -requirements, ownership, state advancement, and return values. +For a long BLAS-like interface, put structured reStructuredText inside +`@verbatim embed:rst:leading-slashes`. +The established parameter form is `name - [direction]`, followed by indented +bullets: ```cpp -/// Fill a dense sketching operator without mutating `state`. +// ============================================================================= +/// Apply a small mathematical operation. /// -/// @tparam T Matrix scalar type. -/// @tparam RNG Random123 counter-based generator type. -/// @param[in] dist Distribution and dimensions to sample. -/// @param[out] buff Destination matrix buffer. -/// @param[in] state Initial random state. -/// @returns the state immediately after the sampled matrix. -``` - -The repository strongly favors `///` over `/** ... */` for header prose. -The `@file` tag is optional. -See [Source documentation](STYLE_AUDIT.md#source-documentation). - -Do not narrate obvious syntax. -Comments should explain facts a reader cannot recover locally, such as: - -- why random output cannot depend on thread count; -- how a CSR/CSC transpose view changes dispatch; -- why a loop order is faster for one layout; -- which matrix a “left” or “right” name describes; -- whether floating-point reassociation is intentional. - -Put public tutorials and API pages under `rtd/source/` in reStructuredText. -Use `:math:` and the project's math macros consistently. -Put implementation rationale in the nearest `DevNotes.md`, and update it when -a dispatch or ownership design changes. - -## 9. Parallel and performance-sensitive code - -Sampling code must remain independent of `OMP_NUM_THREADS`. -Partition counter ranges by logical matrix location, not by a thread's -arrival order. -Add or run thread-count-independence tests whenever sampling changes. - -OpenMP pragmas should sit immediately before the loop they control. -Use explicit scheduling or private/reduction clauses when correctness or -reproducibility depends on them: - -```cpp -#pragma omp parallel for schedule(static) -for (int64_t row = 0; row < n_rows; ++row) { - // Each row receives a deterministic counter range. +/// @verbatim embed:rst:leading-slashes +/// .. dropdown:: Full parameter descriptions +/// :animate: fade-in-slide-down +/// +/// a - [in] +/// * A positive integer. +/// +/// b - [in, out] +/// * On entry: an integer. +/// * On exit: a value determined by :math:`a` and its old value. +/// @endverbatim +void mathfunc(int a, int &b) { + // ... } ``` -Sparse dispatch changes must account for matrix format, transpose flags, and -dense layout. -`left_spmm` has twelve principal paths, and `right_spmm` transforms into that -dispatcher. -Tests must cover every affected path rather than only the easiest storage -format. +Do not put a blank documentation line between `@endverbatim` and the +declaration. +Do not use `@tparam`, `@param`, `@return`, or `@returns` for new web-facing +comments; their Breathe rendering is not the project's chosen presentation. +Existing uses are legacy examples, not a second convention. -Ask for agreement before undertaking a performance optimization. -Benchmark the old and new implementation under comparable settings, then -record the result and relevant BLAS/OpenMP configuration in the change -description. +Outside an embedded reStructuredText block, write inline mathematics with the +project's `\math{...}` alias. +Put the surrounding sentence punctuation inside the alias: `\math{n > 0.}` +Inside an embedded block or an `.rst` file, use reStructuredText math roles and +directives. -## 10. Tests - -Put a test at the same abstraction level as the behavior: - -- basic RNG and statistical behavior in `test/basic_rng/`; -- matrix and sketch-operator types in `test/datastructures/`; -- low-level and wrapper operations in `test/linops/`; -- reusable test support in `RandBLAS/testing/` or a nearby helper header. - -Use a `Test...` fixture and a snake_case case name when shared setup is -useful: +A comment may define a local math macro at the start of an embedded block: ```cpp -class TestDenseSampling : public ::testing::Test { -}; - -TEST_F(TestDenseSampling, output_is_thread_count_independent) { - // Arrange, act, and compare deterministic buffers. -} +/// @verbatim embed:rst:leading-slashes +/// +/// .. |vals| mathmacro:: \mathtt{vals} +/// +/// @endverbatim ``` -Use `ASSERT_*` when failure makes later statements unsafe. -Use `EXPECT_*` for independent comparisons that can continue after one -failure. +Prefer a definition under `rtd/source/` when several API entries or pages need +the same macro. -RNG changes must have deterministic reference checks and, when distributional -behavior changes, appropriate statistical tests. -New sketching operations should exercise left and right application, -transposition, layouts, submatrices, and relevant sparse formats. +Use a line of equals signs to separate major documented declarations and a +line of hyphens for members within a type. +Use `// MARK: description` to make a long source or test file easier to +navigate. -After a code change, run the focused test while iterating and the full suite -before completion: +### Other documentation -```bash -cd /path/to/randnla/dev -source sourceme.sh -ctest --test-dir build-randblas --output-on-failure -``` +Tutorials, policy prose, and API-reference composition belong under +`rtd/source/` and use reStructuredText. +Implementation rationale belongs in the nearest `DevNotes.md`. +Update the relevant API-reference directive when a public declaration changes. + +## Other files -## 11. Examples, CMake, Python, and automation - -Examples should teach a real RandBLAS use or measure a named kernel. -Keep argument parsing, data preparation, correctness checks, and timing code -separate enough that a reader can find the RandBLAS call. -Local timing or matrix-index macros may be used when they make the numerical -code clearer, but they should be narrow and explicitly undefined. - -CMake commands should be lower-case and blocks should use four-space -indentation. -Retain the spelling of public/cache variables such as `BUILD_TESTS` and -`RandBLAS_HAS_OpenMP`. -Prefix new private helper variables and functions with `_rb_`. -The [CMake profile](STYLE_AUDIT.md#cmake-python-and-automation) found 398 of -400 commands in lower case. - -Recommendation: use four-space indentation and conventional import grouping -in Python. -The repository has only three Python files and does not establish a reliable -quote-style rule. - -Shell, PowerShell, YAML, JavaScript, and CSS should follow their file's local -shape. -Automation should favor explicit platform and dependency names over clever -shell compression. - -## 12. Intentional exceptions - -Two files have narrow, documented reasons to differ: - -- `RandBLAS.hh` is the installed umbrella header. - It keeps its include guard and angle-bracket installed paths. -- `test/basic_rng/test_r123.cc` is adapted from the official Random123 test - suite. - Its upstream license, macro-heavy compatibility code, and several spacing - choices should not be copied into ordinary RandBLAS tests. - -An exception may preserve upstream provenance, generated syntax, a public -compatibility surface, or a measured kernel requirement. -Document the reason next to the code or in the nearest DevNotes. -Keep the exception as small as possible. - -## 13. Contributor checklist - -Before requesting review, check the following: - -- [ ] The change uses four-space indentation, same-line braces, and - `template <...>` spacing in new C++. -- [ ] New headers carry the project license, one `#pragma once`, and their - direct dependencies. -- [ ] Public names, BLAS++ flags, dimensions, strides, and validation match - neighboring APIs. -- [ ] Doxygen and DevNotes describe any changed contract, ownership rule, or - dispatch design. -- [ ] Sampling output is unchanged across OpenMP thread counts. -- [ ] Sparse changes cover every affected format/transpose/layout path. -- [ ] Focused tests cover the behavior and failure modes. -- [ ] Performance changes include comparable before/after measurements. -- [ ] `source sourceme.sh; ctest --test-dir build-randblas` passes from the - RandNLA workspace. -- [ ] The patch contains no unrelated formatting sweep. +Use a `Test...` fixture when several tests share setup, and give test cases +snake_case names. +For CMake and the repository's smaller language strata, follow the local file. +Use lower-case CMake command names.