Optimize maybe-used diag_record

It turns out the std::ostringstream member of butl::diag_record is quite
expensive to construct but to never use.

20 files changed, 269 insertions, 161 deletions

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index d2d1eb6..d1cd474 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -861,7 +861,7 @@ namespace build2
               //
               bool ambig (false);
 
-              diag_record dr;
+              maybe_diag_record dr;
               for (++i; i != rs.second; ++i)
               {
                 const rule_match* r1 (&*i);
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index c9193ff..0199137 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -106,7 +106,7 @@ namespace build2
         // name from the script operation first.
         //
         {
-          diag_record dr;
+          maybe_diag_record dr;
 
           if (!diag_name_ && diag_preamble_.empty ())
           {
@@ -131,7 +131,7 @@ namespace build2
                << "'";
           }
 
-          if (!dr.empty ())
+          if (dr)
           {
             dr << info << "consider specifying it explicitly with the 'diag' "
                << "recipe attribute";
@@ -2507,8 +2507,8 @@ namespace build2
             //
             bool df (!ctx.match_only && !ctx.dry_run_option);
 
-            diag_record dr;
-            dr << error << what << ' ' << f << " not found and no rule to "
+            diag_record dr (error);
+            dr << what << ' ' << f << " not found and no rule to "
                << "generate it";
 
             if (df)
diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx
index c8955bc..53d38ac 100644
--- a/libbuild2/cc/compile-rule.cxx
+++ b/libbuild2/cc/compile-rule.cxx
@@ -2350,9 +2350,9 @@ namespace build2
 
                 if (verb > 2)
                 {
-                  diag_record dr;
-                  dr << error << "header " << f << " not found and no "
-                     << "rule to generate it";
+                  diag_record dr (error);
+                  dr << "header " << f << " not found and no rule to "
+                     << "generate it";
 
                   if (verb < 4)
                     dr << info << "re-run with --verbose=4 for more information";
@@ -2925,8 +2925,8 @@ namespace build2
 
                 if (verb > 2)
                 {
-                  diag_record dr;
-                  dr << error << "header '" << f << "' not found";
+                  diag_record dr (error);
+                  dr << "header '" << f << "' not found";
 
                   if (verb < 4)
                     dr << info << "re-run with --verbose=4 for more information";
@@ -4060,9 +4060,8 @@ namespace build2
           //
           bool df (!ctx.match_only && !ctx.dry_run_option);
 
-          diag_record dr;
-          dr << error << "header " << h << " not found and no rule to "
-             << "generate it";
+          diag_record dr (error);
+          dr << "header " << h << " not found and no rule to generate it";
 
           if (df)
             dr << info << "failure deferred to compiler diagnostics";
@@ -4134,9 +4133,8 @@ namespace build2
           //
           bool df (!ctx.match_only && !ctx.dry_run_option);
 
-          diag_record dr;
-          dr << error << "header " << hp << " not found and no rule to "
-             << "generate it";
+          diag_record dr (error);
+          dr << "header " << hp << " not found and no rule to generate it";
 
           if (df)
             dr << info << "failure deferred to compiler diagnostics";
@@ -4985,7 +4983,7 @@ namespace build2
               if (pr.wait ())
               {
                 {
-                  diag_record dr;
+                  maybe_diag_record dr;
 
                   if (bad_error)
                     dr << fail << "expected error exit status from "
@@ -5093,7 +5091,7 @@ namespace build2
                 // preprocessed source files).
                 //
                 {
-                  diag_record dr;
+                  maybe_diag_record dr;
                   if (force_gen_skip && *force_gen_skip == skip_count)
                   {
                     dr <<
@@ -5101,11 +5099,11 @@ namespace build2
                       info << "run the following two commands to investigate";
 
                     dr << info;
-                    print_process (dr, args.data ()); // No pipes.
+                    print_process (*dr, args.data ()); // No pipes.
 
                     init_args ((gen = true));
                     dr << info << "";
-                    print_process (dr, args.data ()); // No pipes.
+                    print_process (*dr, args.data ()); // No pipes.
                   }
 
                   if (dbuf.is_open ())
diff --git a/libbuild2/cc/msvc.cxx b/libbuild2/cc/msvc.cxx
index 416df36..66223b5 100644
--- a/libbuild2/cc/msvc.cxx
+++ b/libbuild2/cc/msvc.cxx
@@ -511,8 +511,8 @@ namespace build2
 
       if (!run_finish_code (args, pr, s, 2 /* verbosity */) || io)
       {
-        diag_record dr;
-        dr << warn << "unable to detect " << l << " library type, ignoring" <<
+        diag_record dr (warn);
+        dr << "unable to detect " << l << " library type, ignoring" <<
           info << "run the following command to investigate" <<
           info; print_process (dr, args);
         return otype::e;
diff --git a/libbuild2/config/operation.cxx b/libbuild2/config/operation.cxx
index 77a1294..5c06f3c 100644
--- a/libbuild2/config/operation.cxx
+++ b/libbuild2/config/operation.cxx
@@ -302,8 +302,8 @@ namespace build2
                   continue;
               }
 
-              diag_record dr;
-              dr << warn (on) << "saving no longer used variable " << *var;
+              diag_record dr (warn (on));
+              dr << "saving no longer used variable " << *var;
               info_import (dr, var->name);
               if (verb >= 2)
                 info_value (dr, v);
@@ -313,8 +313,8 @@ namespace build2
           {
             if (r.second) // warn
             {
-              diag_record dr;
-              dr << warn (on) << "dropping no longer used variable " << *var;
+              diag_record dr (warn (on));
+              dr << "dropping no longer used variable " << *var;
               info_import (dr, var->name);
               info_value (dr, v);
             }
@@ -479,9 +479,8 @@ namespace build2
                 //
                 if (checked && org == ovr)
                 {
-                  diag_record dr;
-                  dr << warn (on) << "saving previously inherited variable "
-                     << var;
+                  diag_record dr (warn (on));
+                  dr << "saving previously inherited variable " << var;
 
                   dr << info << "because project " << *r << " no longer uses "
                      << "it in its configuration";
diff --git a/libbuild2/diagnostics.cxx b/libbuild2/diagnostics.cxx
index c795e44..fdd276e 100644
--- a/libbuild2/diagnostics.cxx
+++ b/libbuild2/diagnostics.cxx
@@ -836,7 +836,7 @@ namespace build2
     // is inseparable from any buffered diagnostics. So we prepare the record
     // first and then write both while holding the diagnostics stream lock.
     //
-    diag_record dr;
+    maybe_diag_record dr;
     if (!pe)
     {
       // Note: see similar code in run_finish_impl().
@@ -852,7 +852,7 @@ namespace build2
         if (verb >= 1 && verb <= v)
         {
           dr << info << "command line: ";
-          print_process (dr, args);
+          print_process (*dr, args);
         }
       }
     }
@@ -861,7 +861,7 @@ namespace build2
   }
 
   void diag_buffer::
-  close (diag_record&& dr)
+  close (maybe_diag_record&& dr)
   {
     assert (state_ != state::closed);
 
@@ -901,7 +901,7 @@ namespace build2
     args0 = nullptr;
     state_ = state::closed;
 
-    if (!buf.empty () || !dr.empty ())
+    if (!buf.empty () || dr)
     {
       diag_stream_lock l;
 
@@ -911,14 +911,14 @@ namespace build2
         buf.clear ();
       }
 
-      if (!dr.empty ())
-        dr.flush ([] (const butl::diag_record& r)
-                  {
-                    // Similar to default_writer().
-                    //
-                    *diag_stream << r.os.str () << '\n';
-                    diag_stream->flush ();
-                  });
+      if (dr)
+        (*dr).flush ([] (const butl::diag_record& r)
+                     {
+                       // Similar to default_writer().
+                       //
+                       *diag_stream << r.os.str () << '\n';
+                       diag_stream->flush ();
+                     });
       else
         diag_stream->flush ();
     }
diff --git a/libbuild2/diagnostics.hxx b/libbuild2/diagnostics.hxx
index ef41f22..3452ad8 100644
--- a/libbuild2/diagnostics.hxx
+++ b/libbuild2/diagnostics.hxx
@@ -4,6 +4,8 @@
 #ifndef LIBBUILD2_DIAGNOSTICS_HXX
 #define LIBBUILD2_DIAGNOSTICS_HXX
 
+#include <cstddef> // max_align_t
+
 #include <libbutl/diagnostics.hxx>
 
 #include <libbuild2/types.hxx>
@@ -447,15 +449,110 @@ namespace build2
       return *this;
     }
 
-    diag_record () = default;
+    // Use maybe_diag_record below for maybe-used records.
+    //
+    diag_record () = delete;
+
+    // Create empty record that will be used.
+    //
+    explicit
+    diag_record (nullptr_t): butl::diag_record () {}
 
     template <typename B>
     explicit
-    diag_record (const diag_prologue<B>& p): diag_record () { *this << p;}
+    diag_record (const diag_prologue<B>& p)
+        : butl::diag_record ()
+    {
+      *this << p;
+    }
 
     template <typename B>
     explicit
-    diag_record (const diag_mark<B>& m): diag_record () { *this << m;}
+    diag_record (const diag_mark<B>& m)
+        : butl::diag_record ()
+    {
+      *this << m;
+    }
+  };
+
+  // A diag_record wrapper similar to std::optional that allows creating an
+  // uninitialized record that may or may not be used. The initializers are
+  // the diagnostics marks, for example:
+  //
+  // maybe_diag_record dr; // Uninitialized.
+  // dr << fail << "bad";  // Initialized.
+  // if (dr)
+  //   *dr << "extra info";
+  //
+  // The reason we need this is because the std::ostringstream member of
+  // butl::diag_record is quite expensive to construct but to never use.
+  //
+  struct maybe_diag_record
+  {
+    maybe_diag_record (): empty_ (true) {}
+
+    explicit operator bool () const
+    {
+      return !empty_;
+    }
+
+    diag_record&
+    operator* ()
+    {
+      return *reinterpret_cast<diag_record*> (data_);
+    }
+
+    template <typename B>
+    diag_record&
+    operator<< (const diag_mark<B>& m)
+    {
+      diag_record* r;
+
+      if (empty_)
+      {
+        r = new (&data_) diag_record (m);
+        empty_ = false;
+      }
+      else
+        *(r = reinterpret_cast<diag_record*> (data_)) << m;
+
+      return *r;
+    }
+
+    template <typename B>
+    diag_record&
+    operator<< (const diag_prologue<B>& p)
+    {
+      diag_record* r;
+
+      if (empty_)
+      {
+        r = new (&data_) diag_record (p);
+        empty_ = false;
+      }
+      else
+        *(r = reinterpret_cast<diag_record*> (data_)) << p;
+
+      return *r;
+    }
+
+    maybe_diag_record (maybe_diag_record&&) = delete;
+    maybe_diag_record& operator= (maybe_diag_record&&) = delete;
+
+    maybe_diag_record (const maybe_diag_record&) = delete;
+    maybe_diag_record& operator= (const maybe_diag_record&) = delete;
+
+    ~maybe_diag_record () noexcept (false) // butl::diag_record may throw
+    {
+      if (!empty_)
+        reinterpret_cast<diag_record*> (data_)->~diag_record ();
+    }
+
+  private:
+    bool empty_;
+
+    static constexpr size_t size_ = sizeof (diag_record);
+    alignas (std::max_align_t) unsigned char data_[size_];
   };
 
   template <typename B>
@@ -467,7 +564,7 @@ namespace build2
     diag_record
     operator<< (const T& x) const
     {
-      diag_record r;
+      diag_record r (nullptr);
       r.append (this->indent, this->epilogue);
       B::operator() (r);
       r << x;
@@ -981,7 +1078,7 @@ namespace build2
     // function will throw.
     //
     void
-    close (diag_record&& = {});
+    close (maybe_diag_record&& = {});
 
     // Direct access to the underlying stream and buffer for custom processing
     // (see read() above for details).
diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index dbeb47e..9d9dfa4 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -40,9 +40,8 @@ namespace build2
       if (!f)
         return nullopt;
 
-      diag_record dr;
-      dr << fail << what << ' ' << pt << " not found and no rule to "
-         << "generate it";
+      diag_record dr (fail);
+      dr << what << ' ' << pt << " not found and no rule to generate it";
 
       if (verb < 4)
         dr << info << "re-run with --verbose=4 for more information";
@@ -107,9 +106,8 @@ namespace build2
       if (!f)
         return nullopt;
 
-      diag_record dr;
-      dr << fail << what << ' ' << pt << " not found and no rule to "
-         << "generate it";
+      diag_record dr (fail);
+      dr << what << ' ' << pt << " not found and no rule to generate it";
 
       if (verb < 4)
         dr << info << "re-run with --verbose=4 for more information";
@@ -139,7 +137,7 @@ namespace build2
                         action a, const target& t, size_t pts_n,
                         const file& pt)
   {
-    diag_record dr;
+    const char* err (nullptr);
 
     if (pt.matched (a, memory_order_acquire))
     {
@@ -148,7 +146,7 @@ namespace build2
       {
         if (pts_n == 0 || !updated_during_match (a, t, pts_n, pt))
         {
-          dr << fail << what << ' ' << pt << " has non-noop recipe";
+          err = "has non-noop recipe";
         }
       }
     }
@@ -157,12 +155,14 @@ namespace build2
       // Note that this target could not possibly be updated during match
       // since it's not matched.
       //
-      dr << fail << what << ' ' << pt << " is explicitly declared as "
-         << "target and may have non-noop recipe";
+      err = "is explicitly declared as target and may have non-noop recipe";
     }
 
-    if (!dr.empty ())
-      dr << info << "consider listing it as static prerequisite of " << t;
+    if (err != nullptr)
+    {
+      fail << what << ' ' << pt << ' ' << err <<
+        info << "consider listing it as static prerequisite of " << t;
+    }
   }
 
   small_vector<const target_type*, 2> dyndep_rule::
diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index fcd3c7c..14b60bd 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -2004,9 +2004,8 @@ namespace build2
 
           if (!opt)
           {
-            diag_record dr;
-            dr << error (loc) << "invalid metadata signature in " << args[0]
-               << " output" <<
+            diag_record dr (error (loc));
+            dr << "invalid metadata signature in " << args[0] << " output" <<
               info << "expected '" << s << "'";
 
             if (verb >= 1 && verb <= 2)
@@ -2042,8 +2041,8 @@ namespace build2
         //
         if (!opt)
         {
-          diag_record dr;
-          dr << error (loc) << "unable to extract metadata from " << args[0] <<
+          diag_record dr (error (loc));
+          dr << "unable to extract metadata from " << args[0] <<
             info << "process " << args[0] << " " << *pr.exit;
 
           if (verb >= 1 && verb <= 2)
@@ -2696,12 +2695,10 @@ namespace build2
         }
         else
         {
-          diag_record dr;
-          dr << fail (loc) << "unable to determine src_root for imported ";
-          if (proj)
-            dr << *proj;
-          else
-            dr << out_root;
+          diag_record dr (fail (loc));
+          dr << "unable to determine src_root for imported ";
+          if (proj) dr << *proj;
+          else      dr << out_root;
           dr << info << "consider configuring " << out_root;
         }
 
@@ -3279,8 +3276,8 @@ namespace build2
     if (opt || exist)
       return nullptr;
 
-    diag_record dr;
-    dr << fail (loc) << "unable to import target " << pk;
+    diag_record dr (fail (loc));
+    dr << "unable to import target " << pk;
 
     if (proj.empty ())
       dr << info << "consider adding its installation location" <<
@@ -3327,8 +3324,8 @@ namespace build2
             if (opt || exist)
               return nullptr;
 
-            diag_record dr;
-            dr << fail (loc) << "unable to import target " << ns;
+            diag_record dr (fail (loc));
+            dr << "unable to import target " << ns;
             import_suggest (dr, *n.proj, nullptr, string (), meta.has_value ());
           }
         }
@@ -3406,8 +3403,8 @@ namespace build2
     if (opt)
       return names {};
 
-    diag_record dr;
-    dr << fail (loc) << "unable to import target " << n;
+    diag_record dr (fail (loc));
+    dr << "unable to import target " << n;
 
     import_suggest (dr, *n.proj, nullptr /* tt */, n.value, false);
 
diff --git a/libbuild2/function.cxx b/libbuild2/function.cxx
index 3110547..3515c8e 100644
--- a/libbuild2/function.cxx
+++ b/libbuild2/function.cxx
@@ -239,9 +239,9 @@ namespace build2
 
         // No match.
         //
-        diag_record dr;
+        diag_record dr (fail (loc));
 
-        dr << fail (loc) << "unmatched call to "; print_call (dr.os);
+        dr << "unmatched call to "; print_call (dr.os);
 
         if (all_ovls != nullptr)
         {
@@ -284,8 +284,8 @@ namespace build2
       {
         // Ambigous match.
         //
-        diag_record dr;
-        dr << fail (loc) << "ambiguous call to "; print_call (dr.os);
+        diag_record dr (fail (loc));
+        dr << "ambiguous call to "; print_call (dr.os);
 
         for (auto f: ovls)
           dr << info << "candidate: " << *f;
diff --git a/libbuild2/functions-json.cxx b/libbuild2/functions-json.cxx
index e06d9a5..9c59c4b 100644
--- a/libbuild2/functions-json.cxx
+++ b/libbuild2/functions-json.cxx
@@ -295,8 +295,8 @@ namespace build2
       }
       catch (const invalid_json_output& e)
       {
-        diag_record dr;
-        dr << fail << "invalid json value: " << e;
+        diag_record dr (fail);
+        dr << "invalid json value: " << e;
 
         if (e.event)
           dr << info << "while serializing " << to_string (*e.event);
diff --git a/libbuild2/functions-process.cxx b/libbuild2/functions-process.cxx
index 6faa798..bcd91a8 100644
--- a/libbuild2/functions-process.cxx
+++ b/libbuild2/functions-process.cxx
@@ -177,8 +177,8 @@ namespace build2
       // While assuming that the builtin has issued the diagnostics on failure
       // we still print the error message (see process_finish() for details).
       //
-      diag_record dr;
-      dr << fail << "builtin " << bn << " " << process_exit (rs);
+      diag_record dr (fail);
+      dr << "builtin " << bn << " " << process_exit (rs);
 
       if (verb >= 1 && verb <= 2)
       {
diff --git a/libbuild2/operation.cxx b/libbuild2/operation.cxx
index 76118ea..fae1251 100644
--- a/libbuild2/operation.cxx
+++ b/libbuild2/operation.cxx
@@ -1061,8 +1061,8 @@ namespace build2
                 : 0);
       };
 
-      diag_record dr;
-      dr << info << "detected unexecuted matched targets:";
+      diag_record dr (info);
+      dr << "detected unexecuted matched targets:";
 
       for (const auto& pt: ctx.targets)
       {
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index cb1bccc..babe4c9 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -2259,9 +2259,9 @@ namespace build2
 
         if (tt != type::word)
         {
-          diag_record dr;
+          diag_record dr (fail (t));
 
-          dr << fail (t) << "unterminated recipe ";
+          dr << "unterminated recipe ";
           if (kind.empty ()) dr << "block"; else dr << kind << "-block";
 
           dr << info (st) << "recipe ";
@@ -3007,8 +3007,8 @@ namespace build2
           // rule-specific search) can possibly succeed so we can fail now and
           // with a more accurate reason. See import2(names) for background.
           //
-          diag_record dr;
-          dr << fail (ploc) << "unable to import target " << n;
+          diag_record dr (fail (ploc));
+          dr << "unable to import target " << n;
           import_suggest (dr, *n.proj, nullptr, string (), false);
         }
         else
@@ -3963,7 +3963,7 @@ namespace build2
             proj = n.variable ();
         }
 
-        diag_record dr;
+        maybe_diag_record dr;
         do // Breakout loop.
         {
           if (!config)
@@ -4067,7 +4067,7 @@ namespace build2
         }
         while (false);
 
-        if (!dr.empty ())
+        if (dr)
           dr << info << "expected variable name in the 'config[.**]."
              << (proj.empty () ? "<project>" : proj.c_str ()) << ".**' form";
       }
@@ -5763,9 +5763,9 @@ namespace build2
   void parser::
   parse_diag (token& t, type& tt)
   {
-    diag_record dr;
     const location l (get_location (t));
 
+    diag_record dr (nullptr);
     switch (t.value[0])
     {
     case 'f': dr << fail (l); break;
diff --git a/libbuild2/script/parser.cxx b/libbuild2/script/parser.cxx
index a82ccb8..96b454f 100644
--- a/libbuild2/script/parser.cxx
+++ b/libbuild2/script/parser.cxx
@@ -1668,9 +1668,9 @@ namespace build2
 
         if (es > 255)
         {
-          diag_record dr;
+          diag_record dr (fail (l));
 
-          dr << fail (l) << "expected exit status instead of ";
+          dr << "expected exit status instead of ";
           to_stream (dr.os, ns, quote_mode::normal);
 
           dr << info << "exit status is an unsigned integer less than 256";
diff --git a/libbuild2/script/run.cxx b/libbuild2/script/run.cxx
index f8f98c1..b7e8b27 100644
--- a/libbuild2/script/run.cxx
+++ b/libbuild2/script/run.cxx
@@ -1301,7 +1301,6 @@ namespace build2
       // Terminate processes gracefully and set the terminate flag for the
       // pipe commands.
       //
-      diag_record dr;
       for (pipe_command* c (pc); c != nullptr; c = c->prev)
       {
         if (process* p = c->proc)
@@ -1324,6 +1323,10 @@ namespace build2
         c->terminated = true;
       }
 
+      // Delay any failure until we process the entire pipeline.
+      //
+      maybe_diag_record dr;
+
       // Wait a bit for the processes to terminate and kill the remaining
       // ones.
       //
@@ -2547,7 +2550,7 @@ namespace build2
 
           // Verify the exit status and issue the diagnostics on failure.
           //
-          diag_record dr;
+          maybe_diag_record dr;
 
           path pr (cmd_path (cmd));
 
@@ -2563,16 +2566,16 @@ namespace build2
 
             if (c->unread_stdout)
             {
-              dr << "stdout ";
+              *dr << "stdout ";
 
               if (c->unread_stderr)
-                dr << "and ";
+                *dr << "and ";
             }
 
             if (c->unread_stderr)
-              dr << "stderr ";
+              *dr << "stderr ";
 
-            dr << "not closed after exit";
+            *dr << "not closed after exit";
           };
 
           // Fail if the process is terminated due to reaching the deadline.
@@ -2588,7 +2591,7 @@ namespace build2
             if (verb == 1)
             {
               dr << info << "command line: ";
-              print_process (dr, *c->args);
+              print_process (*dr, *c->args);
             }
 
             fail = true;
@@ -2643,23 +2646,23 @@ namespace build2
               dr << error (ll) << w << ' ' << pr << ' ';
 
               if (!exit->normal ())
-                dr << *exit;
+                *dr << *exit;
               else
               {
                 uint16_t ec (exit->code ()); // Make sure printed as integer.
 
                 if (!valid)
                 {
-                  dr << "exit code " << ec << " out of 0-255 range";
+                  *dr << "exit code " << ec << " out of 0-255 range";
                 }
                 else
                 {
                   if (cmd.exit)
-                    dr << "exit code " << ec
-                       << (cmp == exit_comparison::eq ? " != " : " == ")
-                       << exc;
+                    *dr << "exit code " << ec
+                        << (cmp == exit_comparison::eq ? " != " : " == ")
+                        << exc;
                   else
-                    dr << "exited with code " << ec;
+                    *dr << "exited with code " << ec;
                 }
               }
 
@@ -2669,7 +2672,7 @@ namespace build2
               if (verb == 1)
               {
                 dr << info << "command line: ";
-                print_process (dr, *c->args);
+                print_process (*dr, *c->args);
               }
 
               if (non_empty (*c->esp, ll) && avail_on_failure (*c->esp, env))
@@ -2683,7 +2686,7 @@ namespace build2
 
               // Print cached stderr.
               //
-              print_file (dr, *c->esp, ll);
+              print_file (*dr, *c->esp, ll);
             }
             else if (c->unread_stdout || c->unread_stderr)
               unread_output_diag (true /* main_error */);
diff --git a/libbuild2/test/rule.cxx b/libbuild2/test/rule.cxx
index 28eb35b..b3a73ce 100644
--- a/libbuild2/test/rule.cxx
+++ b/libbuild2/test/rule.cxx
@@ -745,7 +745,9 @@ namespace build2
         //
         auto term_pipe = [&timed_wait] (pipe_process* pp)
         {
-          diag_record dr;
+          // Delay the failure until we process the entire pipeline.
+          //
+          maybe_diag_record dr;
 
           // Terminate processes gracefully and set the terminate flag for
           // them.
@@ -962,7 +964,7 @@ namespace build2
             //
             if (!fail)
             {
-              diag_record dr;
+              maybe_diag_record dr;
 
               // Note that there can be a race, so that the process we have
               // terminated due to reaching the deadline has in fact exited
@@ -983,14 +985,14 @@ namespace build2
 
                 if (!pe)
                 {
-                  dr << "terminated: execution timeout expired";
+                  *dr << "terminated: execution timeout expired";
 
                   if (p->unread_stderr)
                     dr << error << "stderr not closed after exit";
                 }
                 else if (!pe->normal () || pe->code () != 0)
                 {
-                   dr << *pe;
+                   *dr << *pe;
 
                   if (p->unread_stderr)
                     dr << error << "stderr not closed after exit";
@@ -999,7 +1001,7 @@ namespace build2
                 {
                   assert (p->unread_stderr);
 
-                  dr << "stderr not closed after exit";
+                  *dr << "stderr not closed after exit";
                 }
 
                 if (verb == 1)
@@ -1009,9 +1011,9 @@ namespace build2
                   for (pipe_process* p (b); p != nullptr; p = p->next)
                   {
                     if (p != b)
-                      dr << " | ";
+                      *dr << " | ";
 
-                    print_process (dr, p->args);
+                    print_process (*dr, p->args);
                   }
                 }
               }
diff --git a/libbuild2/utility.cxx b/libbuild2/utility.cxx
index ae7c9b0..83947b2 100644
--- a/libbuild2/utility.cxx
+++ b/libbuild2/utility.cxx
@@ -364,8 +364,8 @@ namespace build2
       //
       // Note: make sure keep the above trace if decide not to print.
       //
-      diag_record dr;
-      dr << error (loc) << "process " << args[0] << " " << pe;
+      diag_record dr (error (loc));
+      dr << "process " << args[0] << " " << pe;
 
       if (verb >= 1 && verb <= v)
       {
diff --git a/libbuild2/variable.cxx b/libbuild2/variable.cxx
index 0abc360..086fa7a 100644
--- a/libbuild2/variable.cxx
+++ b/libbuild2/variable.cxx
@@ -1872,8 +1872,8 @@ namespace build2
           //
           // Note: the same diagnostics as in $json.serialize().
           //
-          diag_record dr;
-          dr << fail << "invalid json value: " << e;
+          diag_record dr (fail);
+          dr << "invalid json value: " << e;
 
           if (e.event)
             dr << info << "while serializing " << to_string (*e.event);
@@ -2099,8 +2099,8 @@ namespace build2
         // We will likely be trying to interpret a member name as an integer
         // due to the incorrect value type so issue appropriate diagnostics.
         //
-        diag_record dr;
-        dr << fail (sloc) << "invalid json value subscript: " << e;
+        diag_record dr (fail (sloc));
+        dr << "invalid json value subscript: " << e;
 
         if (jv != nullptr && jv->type != json_type::object)
           dr << info << "json value type is " << jv->type;
diff --git a/libbuild2/variable.txx b/libbuild2/variable.txx
index 6e00f89..1d27ec7 100644
--- a/libbuild2/variable.txx
+++ b/libbuild2/variable.txx
@@ -135,30 +135,34 @@ namespace build2
   {
     size_t n (ns.size ());
 
-    diag_record dr;
-    if (value_traits<T>::empty_value ? n <= 1 : n == 1)
+    try
     {
-      try
+      if (value_traits<T>::empty_value ? n <= 1 : n == 1)
       {
+        // Throws invalid argument with complete diagnostics, in which case ns
+        // is not moved from.
+        //
         value_traits<T>::assign (
           v,
           (n == 0
            ? T ()
            : value_traits<T>::convert (move (ns.front ()), nullptr)));
       }
-      catch (const invalid_argument& e)
-      {
-        dr << fail << e;
-      }
+      else
+        throw invalid_argument ("");
     }
-    else
-      dr << fail << "invalid " << value_traits<T>::value_type.name
-         << " value: " << (n == 0 ? "empty" : "multiple names");
-
-    if (!dr.empty ())
+    catch (const invalid_argument& e)
     {
+      diag_record dr (fail);
+
+      if (*e.what () != '\0')
+        dr << e;
+      else
+        dr << "invalid " << value_traits<T>::value_type.name << " value: "
+           << (n == 0 ? "empty value" : "multiple names");
+
       if (var != nullptr)
-        dr << " in variable " << var->name;
+        dr << " in variable " << var->name; // Note: appended to above line.
 
       dr << info << "while converting '" << ns << "'";
     }
@@ -170,30 +174,34 @@ namespace build2
   {
     size_t n (ns.size ());
 
-    diag_record dr;
-    if (value_traits<T>::empty_value ? n <= 1 : n == 1)
+    try
     {
-      try
+      if (value_traits<T>::empty_value ? n <= 1 : n == 1)
       {
+        // Throws invalid argument with complete diagnostics, in which case ns
+        // is not moved from.
+        //
         value_traits<T>::append (
           v,
           (n == 0
            ? T ()
            : value_traits<T>::convert (move (ns.front ()), nullptr)));
       }
-      catch (const invalid_argument& e)
-      {
-        dr << fail << e;
-      }
+      else
+        throw invalid_argument ("");
     }
-    else
-      dr << fail << "invalid " << value_traits<T>::value_type.name
-         << " value: " << (n == 0 ? "empty" : "multiple names");
-
-    if (!dr.empty ())
+    catch (const invalid_argument& e)
     {
+      diag_record dr (fail);
+
+      if (*e.what () != '\0')
+        dr << e;
+      else
+        dr << "invalid " << value_traits<T>::value_type.name << " value: "
+           << (n == 0 ? "empty value" : "multiple names");
+
       if (var != nullptr)
-        dr << " in variable " << var->name;
+        dr << " in variable " << var->name; // Note: appended to above line.
 
       dr << info << "while converting '" << ns << "'";
     }
@@ -205,30 +213,34 @@ namespace build2
   {
     size_t n (ns.size ());
 
-    diag_record dr;
-    if (value_traits<T>::empty_value ? n <= 1 : n == 1)
+    try
     {
-      try
+      if (value_traits<T>::empty_value ? n <= 1 : n == 1)
       {
+        // Throws invalid argument with complete diagnostics, in which case ns
+        // is not moved from.
+        //
         value_traits<T>::prepend (
           v,
           (n == 0
            ? T ()
            : value_traits<T>::convert (move (ns.front ()), nullptr)));
       }
-      catch (const invalid_argument& e)
-      {
-        dr << fail << e;
-      }
+      else
+        throw invalid_argument ("");
     }
-    else
-      dr << fail << "invalid " << value_traits<T>::value_type.name
-         << " value: " << (n == 0 ? "empty" : "multiple names");
-
-    if (!dr.empty ())
+    catch (const invalid_argument& e)
     {
+      diag_record dr (fail);
+
+      if (*e.what () != '\0')
+        dr << e;
+      else
+        dr << "invalid " << value_traits<T>::value_type.name << " value: "
+           << (n == 0 ? "empty value" : "multiple names");
+
       if (var != nullptr)
-        dr << " in variable " << var->name;
+        dr << " in variable " << var->name; // Note: appended to above line.
 
       dr << info << "while converting '" << ns << "'";
     }