docs/clean-code.md

   1 Clean Code Best Practices
   2 =========================
   3
   4 This document describes some of the practices we use to maintain high code
   5 quality.
   6
   7 Debug-only build variables
   8 --------------------------
   9
  10 If you have any variables that are used only in a debug build, don't leave
  11 them unused outside of debug builds.
  12
  13 The following is *bad* because it leaves an unused variable, which forces
  14 the entire build to use `-Wno-unused-variable`:
  15
  16 ```
  17 int foobar(int arg)
  18 {
  19         int foo;
  20
  21 #ifdef DEBUG
  22         foo = checkarg(arg);
  23         if (foo != 42)
  24                 return -1;
  25 #endif
  26
  27         bar(arg);
  28
  29         return 0;
  30 }
  31 ```
  32
  33 To solve it, make the definition of `foo` part of the `#ifdef` or in this
  34 case eliminate it completely and check the return value of `checkarg`
  35 directly:
  36
  37 ```
  38 int foobar(int arg)
  39 {
  40 #ifdef DEBUG
  41         if (checkarg(arg) != 42)
  42                 return -1;
  43 #endif
  44
  45         bar(arg);
  46
  47         return 0;
  48 }
  49 ```
  50
  51 ISA & platform #ifdef (ab)use
  52 -----------------------------
  53
  54 At times it is tempting to use ISA and platform defines (e.g., `__amd64`,
  55 `__i386`, and `__x86`) to conditionally compile code.  Before you do so,
  56 use the following questions to guide your determination if using these
  57 defines is use or abuse.
  58
  59 1. Is the feature/driver/code/etc. cross-platform?
  60 2. Is it cross-platform in theory but not fully implemented everywhere?
  61
  62 A 'yes' answer to any of these means that using ISA or platform defs is
  63 abuse and you should add a config variable instead.
  64
  65 For example, support for Linux core files in libproc is implemented only for
  66 x86 but in theory it is a cross-platform feature.  As a result libproc uses
  67 `CONFIG_LINUX_CORE_SUPPORT` to no-op the feature on sparc.  At the same
  68 time while building on x86, the code needs to decide which register names to
  69 use so it also has a `#ifdef __amd64`.  (Note that this check is inside a
  70 code block guarded by `CONFIG_LINUX_CORE_SUPPORT`.)
  71
  72 ### `__i386 || __amd64` vs. `__x86`
  73
  74 Throughout the code you will see checks such as:
  75
  76 ```
  77 #if defined(__i386) || defined(__amd64)
  78 ```
  79
  80 and
  81
  82 ```
  83 #ifdef __x86
  84 ```
  85
  86 While they may appear to be identical, they are not equally expressive.
  87 (Note, *currently* `__x86` is defined as essentially `__i386 || __amd64`
  88 which is why developers get confused.)
  89
  90 * `__i386` means 32-bit x86 ISA.
  91 * `__amd64` means 64-bit x86 ISA.
  92 * `__x86` means any x86 ISA.
  93
  94 Given these definitions, checking for `__x86` reads as "the following code
  95 is meant for *any* x86 ISA,"  while checking for `__i386 || __amd64` reads
  96 as "the following code is meant for 32-bit or 64-bit x86 ISA, but it may
  97 break on other (future) x86 ISAs."
  98
  99 While there are currently only two x86 ISAs, use the expression which better
 100 describes the condition you actually want to check for.  It makes the code
 101 more future proof, and it lets future developers know what the true intent
 102 was.