docs: add "on bit flags" section (#237)

ARCHITECTURE.md

@@ -29,19 +29,23 @@ Pay attention to the "**Architecture Invariant**" sections.
 library must be opt-in through a feature flag called `std` that is enabled by default. When the `alloc` crate is optional,
 a feature flag called `alloc` must exist to enable its use.
 
+**Architectural Invariant**: no platform-dependant code (`#[cfg(windows)]` and such).
+
 **Architectural Invariant**: no non-essential dependency is allowed.
 
 **Architectural Invariant**: no proc-macro dependency. Dependencies such as `syn` should be pushed
 as far as possible from the foundational crates so it doesn’t become too much of a compilation
-bottleneck. The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability,
+bottleneck. [Compilation time is a multiplier for everything][why-care-about-build-time].
+The paper [Developer Productivity For Humans, Part 4: Build Latency, Predictability,
 and Developer Productivity][developer-productivity] by Ciera Jaspan and Collin Green, Google
-researchers, elaborates on why it is important to keep build times low.
+researchers, also elaborates on why it is important to keep build times low.
 
 **Architectural Invariant**: unless the performance, usability or ergonomic gain is really worth
 it, the amount of [monomorphization] incured in downstream user code should be minimal to avoid
 binary bloating and to keep the compilation as parallel as possible. Large generic functions should
 be avoided if possible.
 
+[why-care-about-build-time]: https://matklad.github.io/2021/09/04/fast-rust-builds.html#Why-Care-About-Build-Times
 [developer-productivity]: https://www.computer.org/csdl/magazine/so/2023/04/10176199/1OAJyfknInm
 [monomorphization]: https://rustc-dev-guide.rust-lang.org/backend/monomorph.html
 
@@ -51,14 +55,14 @@ Meta crate re-exporting important crates.
 
 **Architectural Invariant**: this crate re-exports other crates and does not provide anything else.
 
-_TODO_: clean up the dependencies
-
 #### [`crates/ironrdp-pdu`](./crates/ironrdp-pdu)
 
 PDU encoding and decoding.
 
 _TODO_: talk about important types and traits such as PduDecode, PduEncode…
 
+_TODO_: clean up the dependencies
+
 #### [`crates/ironrdp-graphics`](./crates/ironrdp-graphics)
 
 Image processing primitives.

crates/ironrdp-pdu/README.md

@@ -2,7 +2,14 @@
 
 RDP PDU encoding and decoding library.
 
-## WIP: overview of encoding and decoding traits
+- [Overview of encoding and decoding traits](#overview-of-encoding-and-decoding-traits)
+- [Difference between `WriteBuf` and `WriteCursor`](#difference-between-writebuf-and-writecursor)
+- [Difference between `WriteBuf` and `Vec<u8>`](#difference-between-writebuf-and-vecu8)
+- [Most PDUs are "plain old data" structures with public fields](#most-pdus-are-plain-old-data-structures-with-public-fields)
+- [Enumeration-like types should allow resilient parsing](#enumeration-like-types-should-allow-resilient-parsing)
+- [On bit flags](#on-bit-flags)
+
+## Overview of encoding and decoding traits
 
 It’s important for `PduEncode` to be object-safe in order to enable patterns such as the one
 found in `ironrdp-svc`:
@@ -400,3 +407,68 @@ MUST always be rejected. In such cases, an `enum` is deemed appropriate.
 [RangedU32_get]: https://docs.rs/deranged/0.3.8/deranged/struct.RangedU32.html#method.get
 [deranged]: https://docs.rs/deranged/0.3.8/
 [http-status-code]: https://docs.rs/http/0.2.9/http/status/struct.StatusCode.html
+
+## On bit flags
+
+The **TL;DR** is:
+
+- Use **both** `from_bits_retain` and `const _ = !0` when resilient parsing is required.
+    - `const _ = !0` ensures we don’t accidentally have non resilient or destructive parsing. In
+        addition to that, generated methods such as `complement` (`!`) will consider additional bits
+        and follow the principle of least surprise (`!!flags == flags`).
+    - `from_bits_retain` makes it clear at the call site that preserving all the bits is intentional.
+- Use `from_bits` WITHOUT `const _ = !0` when strictness is required (almost never in IronRDP), and
+    document why with an in-source comment.
+
+Bit flags are used quite pervasively in the RDP protocol.
+IronRDP is relying on the [`bitflags` crate][bitflags] to generate well-defined flags structures,
+freeing us from worrying about bitwise logic.
+
+Three notable methods generated by the `bitflags` crate are:
+
+- [`from_bits`][from_bits],
+- [`from_bits_truncate`][from_bits_truncate], and
+- [`from_bits_retain`][from_bits_retain].
+
+Within IronRDP codebase, `from_bits_retain` should generally be used over `from_bits`, and
+`from_bits_truncate` is likely wrong. `from_bits_retain` will simply ignore unknown bits, but will
+not unset them (unlike `from_bits_truncate`), i.e.: the underlying `u32` is set exactly to the value
+received from the network.
+
+Rationale is:
+
+- PDU decoding and encoding logic should uphold the round-tripping property (`m = encode(decode(m))`),
+  and for this property to hold, parsing must be non-destructive (i.e.: lossless),
+  but `from_bits_truncate` is destructive (unknown bits are discarded).
+- Resilient parsing is generally preferred, ignoring unknown values as long as they are not needed and/or as
+  long as ignoring them is causing no harm, but `from_bits` is not lenient
+
+Note that it’s okay to use `from_bits` if strictness is actually required somewhere, but such places must be
+documented with a comment explaining why refusing unknown flags is better.
+
+`bitflags` v2.4 also introduced a new syntax in the `bitflags!` macro (<https://github.com/bitflags/bitflags/pull/371>):
+
+```rust
+bitflags! {
+    pub struct Flags: u32 {
+        const A = 0b00000001;
+        const B = 0b00000010;
+        const C = 0b00000100;
+
+        // The source may set any bits
+        const _ = !0; // <- This
+    }
+}
+```
+
+There is crate-level documentation for this: <https://docs.rs/bitflags/2.4.0/bitflags/#externally-defined-flags>
+
+This addition makes `from_bits_truncate` behave exactly like `from_bits_retain`, because all values
+are considered to be known and defined. In such cases, `from_bits` also never fails therefore works
+precisely the same as `from_bits_retain`, except it’s less ergonomic because it returns a `Result`
+which must be needlessly handled.
+
+[bitflags]: https://crates.io/crates/bitflags
+[from_bits]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits
+[from_bits_truncate]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits_truncate
+[from_bits_retain]: https://docs.rs/bitflags/2.4.0/bitflags/example_generated/struct.Flags.html#method.from_bits_retain