diff options
Diffstat (limited to 'CONTRIBUTING.rst')
| -rw-r--r-- | CONTRIBUTING.rst | 531 |
1 files changed, 531 insertions, 0 deletions
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst new file mode 100644 index 00000000..a0bfd8f8 --- /dev/null +++ b/CONTRIBUTING.rst @@ -0,0 +1,531 @@ +.. sectnum:: + +TeachOS C++ Code Style Guide +============================ + +This document codifies the C++ coding idioms used throughout the TeachOS kernel. +It covers language usage, ownership and lifetime models, algorithm selection, error propagation, class design, and naming conventions. +It does **not** cover token-level formatting, which is enforced automatically by the `.clang-format` configuration. + +Language Standard and Vocabulary +-------------------------------- + +TeachOS targets **C++23** without compiler extensions. +Standard library features may be used freely in tests only. +All other parts of the codebase rely on a freestanding variant of the standard library as shipped with the toolchain. +Types that are not part of the toolchain's freestanding standard library are provided by the `kstd` library. +Below is an overview of `kstd` replacements to be used in the **non-test** kernel code, including the bundled support libraries. +This list is does not claim completeness. + + +=================== ========================= ======================================= +Concept Preferred Replaces +=================== ========================= ======================================= +Dynamic array ``kstd::vector<T>`` ``std::vector<T>`` +Strings ``kstd::string`` ``std::string`` +Non-owning pointer ``kstd::observer_ptr<T>`` raw ``T*`` for ownership-neutral access +Shared ownership ``kstd::shared_ptr<T>`` ``std::shared_ptr<T>`` +Unique ownership ``kstd::unique_ptr<T>`` ``std::unique_ptr<T>`` +Printing ``kstd::println(...)`` ``std::println(...)``, ... +String Formatting ``kstd::format(...)`` ``std::format(...)``, ... +=================== ========================= ======================================= + +Common standard library parts that are available in the freestanding implementation include but are not limited to: + +- ``std::string_view`` +- ``std::span`` +- ``std::array`` +- ``std::optional`` +- ``std::byte`` +- ``std::ranges`` +- ``std::views`` + +These parts may be used freely throughout the codebase. + +Function Declarations — Trailing Return Types +--------------------------------------------- + +**All** functions and member functions use trailing return type syntax, including those returning `void`. +See the following code snippet for examples: + +.. code-block:: cpp + + // Correct + auto device_registry::get() -> device_registry &; + auto bitmap_is_set(std::span<std::byte const> bitmap, std::size_t index) -> bool; + auto init() -> void; + + // Wrong + device_registry & device_registry::get(); + bool bitmap_is_set(std::span<std::byte const> bitmap, std::size_t index); + void init(); + +This rule applies to: free functions, member functions, lambdas with explicit return types, and virtual functions. +The only exception is constructors and destructors, which have no return type at all. + +Parameter Passing Conventions +----------------------------- + +The choice of passing convention encodes intent and must be consistent. + +View and cheaply copyable types — pass by value +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Types that are designed to be non-owning views or are trivially copyable must be passed and returned **by value**. +Passing them by ``const &`` is redundant and adds a pointer indirection with no benefit. + +This includes but is not limited to: + +- ``std::string_view`` +- ``std::span<T>`` +- ``kstd::observer_ptr<T>`` +- ``kstd::bytes`` and similar unit types +- ``kapi::capabilities::facet_id`` +- ``kapi::memory::page`` +- ``kapi::memory::frame`` +- ``kapi::memory::physical_address`` +- ``kapi::memory::linear_address`` + +See the following code snippet for examples: + +.. code-block:: cpp + + // Correct + auto resolve(kapi::capabilities::facet_id id, std::string_view name) -> std::observer_ptr<void>; + auto has(std::span<std::byte const> data) -> bool; + + // Wrong + auto resolve(kapi::capabilities::facet_id const & id, std::string_view const & name) -> std::observer_ptr<void>; + +Large or non-trivially copyable types — pass by ``const &`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +For types that own heap memory or are non-trivially copyable, use ``const &`` when the callee does not take ownership. + +See the following code snippet for examples: + +.. code-block:: cpp + + auto do_publish(kstd::string const & name) -> kstd::result<void>; + auto add_child(kstd::string const & child_name) -> void; + +Sink parameters — pass by value and move +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When a function is designed to take **ownership** of an argument, accept it by value and move it into its destination. +This makes the transfer explicit at the call site. + +See the following code snippet for examples: + +.. code-block:: cpp + + // In the header + auto add_child(kstd::shared_ptr<device> child) -> void; + + // In the implementation + auto bus::add_child(kstd::shared_ptr<device> child) -> void + { + m_devices.push_back(std::move(child)); // ownership transferred here + } + +Mutable subsystem references — pass by non-const reference +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Services and subsystems that are mutated in-place (e.g., ``page_mapper &``, ``driver_state &``, ``kapi::devices::bus &``) are passed by non-const reference. +This expresses that the function operates on a shared, mutable context. + +See the following code snippet for examples: + +.. code-block:: cpp + + auto remap_kernel(kapi::memory::page_mapper & mapper) -> void; + auto add_directory_entry(inode & directory, inode & child, driver_state & state, write_batch & batch) -> kstd::result<void>; + +Error Handling +-------------- + +TeachOS kernel code, and all library code used by it, cannot make use of exceptions. +Use of exception related keywords, `try`, `catch`, `throw` in kernel code will cause compilation to fail. +However, exceptions are allowed in test code. + +Recoverable errors — ``kstd::result<T>`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Functions that can fail in an expected, recoverable way must return ``kstd::result<T>``. +Use the ``kstd::success()`` and ``kstd::failure()`` helpers consistently. +Define per-subsystem error code if necessary. + +See the following code snippet for examples: + +.. code-block:: cpp + + auto mount(kstd::shared_ptr<inode> parent) -> kstd::result<std::pair<kstd::shared_ptr<inode>, state *>>; + + // In the implementation + if (!device) + { + return kstd::failure(make_error_code(kstd::errc::invalid_argument)); + } + return kstd::success(result_value); + +Callers must check the result before using its value. The idiomatic check is: + +See the following code snippet for examples: + +.. code-block:: cpp + + auto result = some_function(); + if (!result) + { + return kstd::failure(result.error()); // propagate + } + // use *result + +Monadic composition (``transform``, ``and_then``, ``or_else``) is preferred over manual if-check-and-return chains when it produces clearer code. + +Unrecoverable errors — ``kapi::system::panic`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Violations of kernel invariants (e.g., a subsystem used before being initialized, OOM during boot) call ``kapi::system::panic(...)``. +The ``panic`` function is ``[[noreturn]]``, meaning control will never return from it. +It must not be used for recoverable errors. + +Log prefix convention: ``[SUBSYSTEM:TAG] message``. Examples: ``[OS:DEV]``, ``[OS:VFS]``, ``[ARCH:DRV]``. + +See the following code snippet for examples: + +.. code-block:: cpp + + if (!instance) + { + system::panic("[OS:DEV] Device registry has not been initialized."); + } + +Ownership and Lifetime +---------------------- + +Shared ownership — ``kstd::shared_ptr`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Use ``kstd::shared_ptr<T>`` when a resource is co-owned by multiple subsystems and its lifetime must be extended by any of them (e.g., ``device``, ``inode``, ``dentry``). +Weak back-references that must not extend lifetime use ``kstd::weak_ptr<T>``. + +Non-owning references — ``kstd::observer_ptr`` and raw references +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Use ``kstd::observer_ptr<T>`` to express a non-owning pointer where null is a valid state and the holder has no say in the lifetime of the target. +Use a raw reference (``T &`` or ``T const &``) when null is not valid and the reference is short-lived (i.e. a function parameter or a local alias). + +Never use raw ``T *`` to mean "sometimes I own this, sometimes I don't". +Ownership must be expressed unambiguously through the pointer type. + +Device driver data — ``kstd::shared_ptr<void>`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Drivers attach their state to a ``device`` using an untyped ``kstd::shared_ptr<void>`` via ``device::set_driver_data``. +A driver retrieves its state via ``device::driver_data``. +This allows the device tree to destroy driver data automatically when the device is released, without the device knowing the concrete driver type. + +Algorithm and Range Usage +------------------------- + +Prefer ``std::ranges`` algorithms over manual loops +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When processing a range to search, filter, transform, or reduce, use the appropriate ``std::ranges`` algorithm or view pipeline instead of writing a raw ``for`` loop. + +See the following code snippet for examples: + +.. code-block:: cpp + + // Correct + auto already_published = std::ranges::any_of(m_entries, [&](auto const & entry) { + return entry.id() == id && entry.device().get() == device.get(); + }); + + std::ranges::for_each(observers, [&](auto observer) { /* ... */ }); + + // Wrong — manual linear scan for a boolean result + for (auto const & entry : m_entries) + { + if (entry.id() == id && entry.device().get() == device.get()) + { + return true; + } + } + return false; + +Acceptable uses of explicit loops include: + +- Accumulation or mutation that modifies state in-place and cannot be cleanly expressed with a ranges algorithm. +- Low-level routines dealing with raw memory arithmetic (allocators, page mappers). +- Iterator-pair loops in library internals (``kstd::vector``, ``kstd::basic_string``). + +Prefer view composition over intermediate containers +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Build processing pipelines using ``std::views::filter``, ``std::views::transform``, ``std::views::reverse``, ``std::views::split``, and ``std::ranges::subrange`` rather than materialising intermediate vectors. + +See the following code snippet for examples: + +.. code-block:: cpp + + // Correct + auto descriptors = std::span{&__start_platform_drivers, &__stop_platform_drivers} + | std::views::filter([](auto p) { return p != nullptr; }); + + auto modules_view = std::ranges::subrange(begin(), end()) + | std::views::filter(filter_modules) + | std::views::transform(transform_module); + +Do not call the same function twice to avoid storing the result +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If an intermediate value is needed more than once, store it in a local variable. +This applies especially to factory calls and heap allocations. + +.. code-block:: cpp + + // Wrong — double invocation, two allocations, different objects + for (auto driver : descriptors) + { + kstd::println("registering driver '{}'", driver->make_instance()->name()); + registry.add(driver->make_instance()); + } + + // Correct + for (auto driver : descriptors) + { + auto instance = driver->make_instance(); + kstd::println("registering driver '{}'", instance->name()); + registry.add(std::move(instance)); + } + +Class and Struct Design +----------------------- + +Prefer ``struct`` over ``class`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The entire codebase uses ``struct`` for all type definitions with explicit ``private:`` +sections where necessary. +Do not introduce ``class``. + +Member ordering within a type +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Follow this ordering within a ``struct``: + +1. Nested types and type aliases. +2. Static data members and static constexpr constants (e.g., ``static constexpr auto id = ...``). +3. Constructors and destructor. +4. Public member functions. +5. ``protected:`` section with virtual hooks. +6. ``private:`` section with helper functions, then data members. + +Data members are always in the ``private`` section and always prefixed with ``m_``. + +See the following code snippet for examples: + +.. code-block:: cpp + + struct facet_registry + { + struct entry { /* ... */ }; // 1. nested type + + facet_registry() = default; // 3. constructor + + auto static init() -> void; // 4. public interface + auto static get() -> facet_registry &; + auto publish(...) -> kstd::result<void>; + + private: + auto do_publish(...) -> kstd::result<void>; // 6a. private helpers + + mutable tracked_mutex m_lock{}; // 6b. data members, m_ prefix + kstd::vector<entry> m_entries; + }; + +``explicit`` on single-argument constructors +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Mark every single-argument constructor ``explicit`` unless an implicit conversion is intentional and documented. +A deliberate implicit constructor must be accompanied by a comment explaining the decision. + +See the following code snippet for examples: + +.. code-block:: cpp + + // Correct + explicit device(kstd::string const & name); + constexpr explicit facet_id(std::string_view name); + + // Intentional implicit — documented at the declaration + //! This constructor allows implicit conversion from chunk<...> to page for + //! convenience. It is deliberately not explicit. + constexpr page(chunk other) : chunk{other} {} + +Declare deleted special members explicitly +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If a type is not copyable or not movable, declare the deleted special members explicitly rather than relying on implicit suppression. + +See the following code snippet for examples: + +.. code-block:: cpp + + device(device const &) = delete; + auto operator=(device const &) -> device & = delete; + +Virtual destructors +~~~~~~~~~~~~~~~~~~~ + +Every base class with virtual member functions must have a ``virtual`` destructor, defaulted if not otherwise needed. + +See the following code snippet for examples: + +.. code-block:: cpp + + virtual ~driver_descriptor() = default; + virtual ~facet_registry_observer() = default; + +Static Singletons +----------------- + +Several subsystems expose a single global instance via an ``init()``/``get()`` pair. +Follow this pattern: + +- Store the instance in an anonymous namespace as a ``constinit std::optional<T>``. +- ``init()`` asserts the instance is not yet constructed, then constructs it using ``emplace()`` it. +- ``get()`` asserts the instance exists and returns a reference to it. +- Both functions panic on violation rather than returning an error code, because incorrect call order is a programming error, not a recoverable runtime condition. + +See the following code snippet for examples: + +.. code-block:: cpp + + namespace + { + auto constinit instance = std::optional<device_registry>{}; + } + + auto device_registry::init() -> void + { + if (instance) + { + system::panic("[OS:DEV] Device registry has already been initialized."); + } + instance.emplace(); + } + + auto device_registry::get() -> device_registry & + { + if (!instance) + { + system::panic("[OS:DEV] Device registry has not been initialized."); + } + return *instance; + } + +Enumerations +------------ + +All enumerations use ``enum struct`` (scoped enums), never plain ``enum``. +Specify the underlying type explicitly when the representation matters (e.g., for hardware register fields). + +See the following code snippet for examples: + +.. code-block:: cpp + + // Correct + enum struct state + { + uninitialized, + present, + bound, + }; + + // Wrong + enum state { uninitialized, present, bound }; + +Naming +------ + +================================== ============================= ===================================== +Symbol Convention Example +================================== ============================= ===================================== +Types (struct, enum) ``lower_case`` ``device_registry``, ``facet_id`` +Functions and methods ``lower_case`` ``add_child``, ``make_instance`` +Local variables ``lower_case`` ``entry``, ``block_index`` +Private data members ``m_`` prefix, ``lower_case`` ``m_entries``, ``m_driver_data`` +Template type parameters ``CamelCase`` ``ValueType``, ``FacetType`` +Constants and constexpr variables ``lower_case`` ``page_size``, ``direct_block_count`` +Type aliases ``lower_case`` ``value_type``, ``size_type`` +Namespaces ``lower_case`` ``kapi::devices``, ``kernel::vfs`` +================================== ============================= ===================================== + + +Namespaces reflect directory structure: ``kernel::vfs``, ``kernel::filesystems::ext2``, ``arch::devices``, etc. + +``[[nodiscard]]`` +----------------- + +Mark any function ``[[nodiscard]]`` whose return value the caller should not silently discard. +This includes in particular: + +- All functions returning ``kstd::result<T>``. +- All query functions (getters, lookups) that return computed data. +- Factory functions and builder utilities. + +See the following code snippet for examples: + +.. code-block:: cpp + + [[nodiscard]] auto children() const -> kstd::vector<kstd::shared_ptr<device>>; + [[nodiscard]] auto resolve(kapi::capabilities::facet_id id, std::string_view name) -> void *; + [[nodiscard]] auto request_resource(resource_type type, std::size_t index = 0) const -> kstd::result<resource>; + +``constexpr`` and ``constinit`` +------------------------------- + +Mark functions ``constexpr`` whenever they can be evaluated at compile time or in constant expressions, even if they are also called at runtime. +Mark module-scope variables ``constinit`` to guarantee zero-initialization before any dynamic initialization runs. + +Documentation +------------- + +Every non-trivial public type, function, and data member must be documented with a Doxygen comment using the ``//!`` line style. + +See the following code snippet for examples: + +.. code-block:: cpp + + //! A brief one-line description. + //! + //! Optional longer description providing context. + //! + //! @warning Any important warnings or preconditions. + //! + //! @param name Description of the parameter. + //! @return Description of the return value. + auto add_child(kstd::shared_ptr<device> child) -> void; + +Doxygen grouping (``@name``, ``@{``, ``@}``) may be used to organize the API surface into logical sections visible in generated documentation. + +Header Guards +------------- + +All headers use traditional include guards, not ``#pragma once``. +The guard name follows the pattern ``TEACHOS_<SUBPACKAGE>_<PATH>_HPP``, where each path component is uppercased and separators are replaced by ``_``. +The only exception to this pattern are the bundled libraries, which follow the pattern ``<LIBRARYNAME>_<SUBPACKAGE>_<PATH>_HPP``. + +See the following code snippet for examples: + +.. code-block:: cpp + + #ifndef TEACHOS_KAPI_DEVICES_BUS_HPP + #define TEACHOS_KAPI_DEVICES_BUS_HPP + // ... + #endif + +The closing ``#endif`` carries no comment with the guard name. |