From b705be95275a192d46ea8b83ddf3b6d104317e38 Mon Sep 17 00:00:00 2001
From: Dana Jansens <danakj@orodu.net>
Date: Tue, 3 Dec 2024 14:01:12 -0500
Subject: [PATCH] Use a Timings* in place of optional<Timings>* (#4607)

The current representation has two nested presence indicatators (the
optional bool, the null pointer). Currently the pointer is never null,
but the style guide suggests that T* should be used for parameters that
may or may not be present, so we do not need the optional here.

> When passing an object's address as an argument, use a reference
> unless one of the following cases applies:
>
> - If the parameter is optional, use a pointer and document that it
>   may be null.

Once the parameter is just a pointer, the ScopedTiming field does not
need an optional either, and can just store the pointer.

It would be more preferable to have an optional representation of a
sometimes-null pointer like optional<T&> to describe a sometimes-null
pointer, as this would allow clearer runtime diagnostics when used
incorrectly (a check failure in unwrapping) and would be better
self-documenting through syntax instead of a comment. But we do not
currently have such a primitive.
---
 toolchain/base/timings.h                | 24 +++++++++++++-----------
 toolchain/check/check.h                 |  3 ++-
 toolchain/driver/compile_subcommand.cpp |  4 ++--
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/toolchain/base/timings.h b/toolchain/base/timings.h
index b1f5795ce567..7da87e292cfa 100644
--- a/toolchain/base/timings.h
+++ b/toolchain/base/timings.h
@@ -5,6 +5,8 @@
 #ifndef CARBON_TOOLCHAIN_BASE_TIMINGS_H_
 #define CARBON_TOOLCHAIN_BASE_TIMINGS_H_
 
+#include <chrono>
+
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "toolchain/base/yaml.h"
@@ -14,26 +16,26 @@ namespace Carbon {
 // Helps track timings for a compile.
 class Timings {
  public:
-  // Records a timing for a scope, if the timings object is valid.
+  // Records a timing for a scope, if the timings object is present.
   class ScopedTiming {
    public:
-    explicit ScopedTiming(std::optional<Timings>* timings,
-                          llvm::StringRef label)
-        : timings(timings),
-          label(label),
-          start(*timings ? std::chrono::steady_clock::now()
+    // The `timings` may be null, in which case the `ScopedTiming` is a no-op.
+    explicit ScopedTiming(Timings* timings, llvm::StringRef label)
+        : timings_(timings),
+          label_(label),
+          start_(timings ? std::chrono::steady_clock::now()
                          : std::chrono::steady_clock::time_point::min()) {}
 
     ~ScopedTiming() {
-      if (*timings) {
-        (*timings)->Add(label, std::chrono::steady_clock::now() - start);
+      if (timings_) {
+        timings_->Add(label_, std::chrono::steady_clock::now() - start_);
       }
     }
 
    private:
-    std::optional<Timings>* timings;
-    llvm::StringRef label;
-    std::chrono::steady_clock::time_point start;
+    Timings* timings_;
+    llvm::StringRef label_;
+    std::chrono::steady_clock::time_point start_;
   };
 
   // Adds tracking for nanoseconds, paired with the given label.
diff --git a/toolchain/check/check.h b/toolchain/check/check.h
index 502aec146c8b..51784f9b4b77 100644
--- a/toolchain/check/check.h
+++ b/toolchain/check/check.h
@@ -21,7 +21,8 @@ namespace Carbon::Check {
 struct Unit {
   DiagnosticConsumer* consumer;
   SharedValueStores* value_stores;
-  std::optional<Timings>* timings;
+  // The `timings` may be null if nothing is to be recorded.
+  Timings* timings;
   const Lex::TokenizedBuffer* tokens;
   const Parse::Tree* parse_tree;
 
diff --git a/toolchain/driver/compile_subcommand.cpp b/toolchain/driver/compile_subcommand.cpp
index 080d8060c92c..219d87ae33cb 100644
--- a/toolchain/driver/compile_subcommand.cpp
+++ b/toolchain/driver/compile_subcommand.cpp
@@ -449,7 +449,7 @@ class CompilationUnit {
     sem_ir_converter_.emplace(node_converters, &*sem_ir_);
     return {.consumer = consumer_,
             .value_stores = &value_stores_,
-            .timings = &timings_,
+            .timings = timings_ ? &*timings_ : nullptr,
             .tokens = &*tokens_,
             .parse_tree = &*parse_tree_,
             .get_parse_tree_and_subtrees = *get_parse_tree_and_subtrees_,
@@ -657,7 +657,7 @@ class CompilationUnit {
                llvm::StringLiteral timing_label, llvm::function_ref<void()> fn)
       -> void {
     CARBON_VLOG("*** {0}: {1} ***\n", logging_label, input_filename_);
-    Timings::ScopedTiming timing(&timings_, timing_label);
+    Timings::ScopedTiming timing(timings_ ? &*timings_ : nullptr, timing_label);
     fn();
     CARBON_VLOG("*** {0} done ***\n", logging_label);
   }