From ad2e6975af00955aa5f265387bd78497aa5a6307 Mon Sep 17 00:00:00 2001
From: DaneBettis <dane.bettis@gmail.com>
Date: Thu, 24 Feb 2022 10:52:56 +0000
Subject: [PATCH] changes in response to code review

---
 pkg/arvo/tests/run/hints.hoon  |  25 +++++---
 pkg/urbit/include/noun/trace.h |  31 +++++-----
 pkg/urbit/noun/nock.c          | 110 +++++++++++++++++----------------
 pkg/urbit/noun/trace.c         |  42 +++++++------
 4 files changed, 111 insertions(+), 97 deletions(-)

diff --git a/pkg/arvo/tests/run/hints.hoon b/pkg/arvo/tests/run/hints.hoon
index f0f9575e4..7481b105b 100644
--- a/pkg/arvo/tests/run/hints.hoon
+++ b/pkg/arvo/tests/run/hints.hoon
@@ -1,16 +1,21 @@
 ::  Test that these hints do not crash the runtime
-::    we only test the dynamic hints here
-::    also, there is no need to include the hints for dynamic %bout
+::    there is no need to include the hints for dynamic %bout
 ::    since all hoon tests exersize dynamic %bout
 |%
-::  this tests that the short trace hint
-::  is safe to run or ignore
-++  test-nara
-  ~>  %nara.[1 leaf+"nara trace hint test"]
+::  these test that the hilt-trace hints
+::  are safe to run or ignore
+++  test-hela-hilt
+  ~>  %hela
   ~
-::  this tests that the full trace hint
-::  is safe to run or ignore
-++  test-hela
-  ~>  %hela.[1 leaf+"hela trace hint test"]
+++  test-nara-hilt
+  ~>  %nara
+  ~
+::  these test that the hint-trace hints
+::  are safe to run or ignore
+++  test-hela-hint
+  ~>  %hela.[1 leaf+"test-hela-trace-hint"]
+  ~
+++  test-nara-hint
+  ~>  %nara.[1 leaf+"test-nara-trace-hint"]
   ~
 --
diff --git a/pkg/urbit/include/noun/trace.h b/pkg/urbit/include/noun/trace.h
index ab83edb9c..bafe1812e 100644
--- a/pkg/urbit/include/noun/trace.h
+++ b/pkg/urbit/include/noun/trace.h
@@ -130,29 +130,30 @@
       void
       u3t_boot(void);
 
-    /* u3t_dynamic_header: slog a vere headline, and hoon hint with
-     * a given indent-style c3_l int (assumed 0-3).
-     */
+    /* u3t_slog_cap(): slog a tank with a caption with
+    ** a given priority c3_l (assumed 0-3).
+    */
       void
-      u3t_dynamic_header(c3_l pri_l, u3_noun headline, u3_noun message);
+      u3t_slog_cap(c3_l pri_l, u3_noun cap, u3_noun tan);
 
-    /* u3t_slog_trace: given an indent-style c3_l int and a raw stack tax
-     * flop the order into start-to-end, render, and slog each item
-     * until done.
-     */
+    /* u3t_slog_trace(): given a c3_l priority pri and a raw stack tax
+    ** flop the order into start-to-end, render, and slog each item
+    ** until done.
+    */
       void
       u3t_slog_trace(c3_l pri_l, u3_noun tax);
 
-    /* u3t_nara: slog only the deepest road's trace
-     */
+    /* u3t_slog_nara(): slog only the deepest road's trace with
+    ** c3_l priority pri
+    */
       void
-      u3t_nara(c3_l pri_l);
+      u3t_slog_nara(c3_l pri_l);
 
-    /* u3t_hela: join all roads' traces together into one tax
-     * and pass it to slog_trace allong with the given pri_l
-     */
+    /* u3t_slog_hela(): join all roads' traces together into one tax
+    ** and pass it to slog_trace along with the given c3_l priority pri_l
+    */
       void
-      u3t_hela(c3_l pri_l);
+      u3t_slog_hela(c3_l pri_l);
 
   /** Globals.
   **/
diff --git a/pkg/urbit/noun/nock.c b/pkg/urbit/noun/nock.c
index b97403fff..e289ed1b7 100644
--- a/pkg/urbit/noun/nock.c
+++ b/pkg/urbit/noun/nock.c
@@ -1698,20 +1698,28 @@ u3n_find(u3_noun key, u3_noun fol)
 static c3_o
 _n_hilt_fore(u3_noun hin, u3_noun bus, u3_noun* out)
 {
-  if ( c3__bout == u3h(hin) ) {
-    u3_atom now = u3i_chub(u3t_trace_time());
-    *out = u3i_cell(u3h(hin), now);
-  }
-  else if ( c3__nara == u3h(hin) ) {
-    u3t_nara(0);
-    *out = u3_nul;
-  }
-  else if ( c3__hela == u3h(hin) ) {
-    u3t_hela(0);
-    *out = u3_nul;
-  }
-  else {
-    *out = u3_nul;
+  u3_noun tag, fol;
+  u3x_cell(hin, &tag, &fol);
+
+  switch ( tag ) {
+    case c3__bout: {
+      u3_atom now = u3i_chub(u3t_trace_time());
+      *out = u3i_cell(tag, now);
+    } break;
+
+    case c3__nara : {
+      u3t_slog_nara(0);
+      *out = u3_nul;
+    } break;
+
+    case c3__hela : {
+      u3t_slog_hela(0);
+      *out = u3_nul;
+    } break;
+
+    default: {
+      *out = u3_nul;
+    } break;
   }
 
   u3z(hin);
@@ -1733,11 +1741,6 @@ _n_hilt_hind(u3_noun tok, u3_noun pro)
     u3t_slog(u3nc(0, u3i_string(str_c)));
     u3z(delta);
   }
-  else if ( (c3y == u3r_cell(tok, &p_tok, &q_tok)) &&
-      ((c3__nara == p_tok) || (c3__hela == p_tok))
-  ) {
-    // DO NOTHING
-  }
   else {
     c3_assert( u3_nul == tok );
   }
@@ -1757,13 +1760,41 @@ _n_hilt_hind(u3_noun tok, u3_noun pro)
 static c3_o
 _n_hint_fore(u3_cell hin, u3_noun bus, u3_noun* clu)
 {
-  if ( c3__bout == u3h(hin) || c3__nara == u3h(hin) || c3__hela == u3h(hin) ) {
-    u3_atom now = u3i_chub(u3t_trace_time());
-    *clu = u3i_trel(u3h(hin), *clu, now);
-  }
-  else {
-    u3z(*clu);
-    *clu = u3_nul;
+  u3_noun tag, fol;
+  u3x_cell(hin, &tag, &fol);
+
+  switch ( tag ) {
+    case c3__bout: {
+      u3_atom now = u3i_chub(u3t_trace_time());
+      *clu = u3nt(u3k(tag), *clu, now);
+    } break;
+
+    case c3__nara: {
+      u3_noun pri, tan;
+      if ( c3y == u3r_cell(*clu, &pri, &tan) ) {
+        c3_l pri_l = c3y == u3a_is_cat(pri) ? pri : 0;
+        u3t_slog_cap(pri_l, u3i_string("trace of"), u3k(tan));
+        u3t_slog_nara(pri_l);
+      }
+      u3z(*clu);
+      *clu = u3_nul;
+    } break;
+
+    case c3__hela: {
+      u3_noun pri, tan;
+      if ( c3y == u3r_cell(*clu, &pri, &tan) ) {
+        c3_l pri_l = c3y == u3a_is_cat(pri) ? pri : 0;
+        u3t_slog_cap(pri_l, u3i_string("trace of"), u3k(tan));
+        u3t_slog_hela(pri_l);
+      }
+      u3z(*clu);
+      *clu = u3_nul;
+    } break;
+
+    default: {
+      u3z(*clu);
+      *clu = u3_nul;
+    } break;
   }
 
   u3z(hin);
@@ -1796,34 +1827,9 @@ _n_hint_hind(u3_noun tok, u3_noun pro)
     // prepend the priority to form a cell of the same shape q_tok
     // send this to ut3_slog so that it can be logged out
     c3_l pri_l = c3y == u3a_is_cat(p_q_tok) ? p_q_tok : 0;
-    u3t_dynamic_header(pri_l, u3k(q_q_tok), u3i_string(str_c));
+    u3t_slog_cap(pri_l, u3k(q_q_tok), u3i_string(str_c));
     u3z(delta);
   }
-  else if ( (c3y == u3r_trel(tok, &p_tok, &q_tok, &r_tok)) &&
-      ((c3__nara == p_tok) || (c3__hela == p_tok))
-  ) {
-    // unpack q_tok to get the priority integer and the tank
-    // p_q_tok is the priority, q_q_tok is the tank we will work with
-    u3_noun p_q_tok, q_q_tok;
-    c3_assert(c3y == u3r_cell(q_tok, &p_q_tok, &q_q_tok));
-
-    // format the timing report
-    c3_c str_c[64];
-    snprintf(str_c, 63, "trace of");
-
-    // join the timing report with the original tank from q_q_tok like so:
-    // "q_q_tok: report"
-    // prepend the priority to form a cell of the same shape q_tok
-    // send this to ut3_slog so that it can be logged out
-    c3_l pri_l = c3y == u3a_is_cat(p_q_tok) ? p_q_tok : 0;
-    u3t_dynamic_header(pri_l, u3i_string(str_c), u3k(q_q_tok));
-    if (c3__nara == p_tok) {
-      u3t_nara(pri_l);
-    }
-    else {
-      u3t_hela(pri_l);
-    }
-  }
   else {
     c3_assert( u3_nul == tok );
   }
diff --git a/pkg/urbit/noun/trace.c b/pkg/urbit/noun/trace.c
index 8ebcb5830..887a1b1c7 100644
--- a/pkg/urbit/noun/trace.c
+++ b/pkg/urbit/noun/trace.c
@@ -590,11 +590,12 @@ u3t_boff(void)
   }
 }
 
-/* where pir_l is the priority, headline is provided by the c-caller
- * and message is provided by the hint-caller
- */
+
+/* u3t_slog_cap(): slog a tank with a caption with
+** a given priority c3_l (assumed 0-3).
+*/
 void
-u3t_dynamic_header(c3_l pri_l, u3_noun headline, u3_noun message)
+u3t_slog_cap(c3_l pri_l, u3_noun cap, u3_noun tan)
 {
   u3t_slog(
     u3nc(
@@ -602,17 +603,19 @@ u3t_dynamic_header(c3_l pri_l, u3_noun headline, u3_noun message)
       u3nt(
         c3__rose,
         u3nt(u3nt(':', ' ', u3_nul), u3_nul, u3_nul),
-        u3nt(headline, message, u3_nul)
+        u3nt(cap, tan, u3_nul)
       )
     )
   );
 }
 
-/* This flops the given tax and slogs it entry by entry.
- */
+
+/* u3t_slog_trace(): given a c3_l priority pri and a raw stack tax
+** flop the order into start-to-end, render, and slog each item
+** until done.
+*/
 void
-u3t_slog_trace
-(c3_l pri_l, u3_noun tax)
+u3t_slog_trace(c3_l pri_l, u3_noun tax)
 {
   // render the stack
   // Note: ton is a reference to a data struct
@@ -636,24 +639,23 @@ u3t_slog_trace
   u3z(ton);
 }
 
-/* this function calls slog_trace on the same road
- * as the wrapped hoon function was called in effectively
- * showing only the short trace from just after the most
- * recent virtualization, down into the wrap site
- * it takes a c3_l pri_l
- */
+
+/* u3t_slog_nara(): slog only the deepest road's trace with
+** c3_l priority pri
+*/
 void
-u3t_nara(c3_l pri_l)
+u3t_slog_nara(c3_l pri_l)
 {
   u3_noun tax = u3k(u3R->bug.tax);
   u3t_slog_trace(pri_l, tax);
 }
 
-/* this function joins all the road traces together
- * into a sinlge trace, which it sends to slog_trace
- */
+
+/* u3t_slog_hela(): join all roads' traces together into one tax
+** and pass it to slog_trace along with the given c3_l priority pri_l
+*/
 void
-u3t_hela(c3_l pri_l)
+u3t_slog_hela(c3_l pri_l)
 {
   // rod_u protects us from mutating the global state
   u3_road* rod_u = u3R;