A junior engineer's first scoreboard arrives for review. It compiles, dma_smoke_test passes with eight words, and the code is tidy. Approving it would take one click. The bench already has a scoreboard that looked just as finished when it was written: dma_predictor and dma_scoreboard take a new KEY at once, while the DMA ignores KEY writes while busy. Nobody's review caught it; it became ticket DMA-142 and a waiver. What does a senior reviewer look for that the compiler, the linter and a passing test cannot?

The idea: review meaning first, mechanics second

Compilers catch what is illegal, linters catch what is legal but known to be wrong, and regressions catch what goes wrong in the cases you happened to run. What none of them catch is whether the code means the right thing: whether the model matches the specification, whether a check can ever fail, and whether a test proves what its name claims. That is the reviewer's job, and it is the part that needs experience. An editor works the same way: a spell checker fixes typos, but only a reader who knows the subject notices that a paragraph is confidently wrong.

Mechanics still matter, but they belong in a checklist and a linter, not in the reviewer's head. Testbench Code Quality has the rules and a per-component checklist. A senior reviewer reads with that checklist in hand and spends their attention on six questions:

The picture: what each layer catches

Build it: reviewing the junior's scoreboard

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24

// bad-example: a junior's first scoreboard, as submitted (it compiles and the smoke test passes)
class my_dma_sb extends uvm_scoreboard;
  `uvm_component_utils(my_dma_sb)
  uvm_analysis_imp_in  #(axis_item, my_dma_sb) in_export;
  uvm_analysis_imp_out #(axis_item, my_dma_sb) out_export;
  bit [31:0] key;
  bit [31:0] expected[$];
  function new(string name, uvm_component parent);
    super.new(name, parent);
    in_export  = new("in_export", this);
    out_export = new("out_export", this);
  endfunction
  function void build_phase(uvm_phase phase);
    void'(uvm_config_db#(bit [31:0])::get(this, "", "key", key));   // (1)
  endfunction                                                      // (2)
  function void write_in(axis_item t);                             // (3)
    expected.push_back(t.data ^ key);
  endfunction
  function void write_out(axis_item t);
    bit [31:0] exp = expected.pop_front();                         // (4)
    if (t.data != exp)                                             // (5)
      `uvm_warning("MISMATCH", "data mismatch")                    // (6)
  endfunction
endclass                                                           // (7)

Here are the review comments, in the order a senior reviewer would write them, marked by whether they block the merge. Note the wording: questions that teach, not verdicts.

Notice what is not on the list: naming, spacing and comment style. If those matter to the team, a formatter enforces them. A review with thirty style nits and these seven comments mixed in teaches the author that review is about style, and the one comment that would have prevented DMA-142 gets lost in the noise. Ask for the evidence too: "please attach a run with +define+DMA_BUG_WORD6 showing this scoreboard catch word 6". It takes the author five minutes and proves the check can fail.

Mentoring a junior through their first scoreboard or agent

• Give a small, real piece with a clear finish line: the TLAST check, not "the scoreboard". The first change should land within days, so the feedback loop is short.

• Ask before you tell. "What happens if the KEY write arrives while the DMA is busy?" teaches the habit of asking that question. Telling them the answer teaches only the answer.

• Make them break it. A planted bug, a removed objection, an override that does not apply. Engineers who have seen their own check fail trust it for the right reasons.

• Pair on the first real debug, then let them drive. Your job is to keep them from rabbit holes: "what does the first error line say?" before any waveform.

• Have them explain their own log in review. "14 words matched" is a number they should be able to account for from the test: three jobs, LEN 10, 1 and 3.

Growing reviewers

A team where only the lead reviews has a bottleneck and a single point of failure. Grow reviewers deliberately. A junior first reviews alongside a senior, writes their comments separately, and compares; the gap between the two lists is the lesson. Then they review alone on low-risk code with a senior approving after them, and finally they own review for a component. Keep a short review checklist in the repository, made from your own incidents (every war story in this course became a line), and update it when a bug escapes review. One useful measure: of the bench bugs found this quarter, how many were caught in review and how many in regression? A rising review share means the team is getting better at it.

What breaks: the review that approved DMA-142

dma_predictor takes a new KEY as soon as the APB monitor reports the write. The DMA's header says, in one line, that KEY and LEN writes are ignored while BUSY. A review that checked the code against the checklist would have passed it: virtual methods, checked casts, clear IDs, a fresh item per prediction. The bug is not in the code; it is between the code and the specification, and only a reviewer who read the spec sentence and asked "when does this input take effect?" would have seen it. No bench test writes KEY in the middle of a job, so the regression never complained either. Months later a scratch run that did gave five mismatches in both checkers. The repair is a better predictor and one directed test; the process repair is the second question in the table above, asked for every input of every model.

Next: reviews keep the bench honest week by week. Sign-off and Tape-out Readiness is the day someone asks whether it is done.

Related topics

• Up: Leadership and Senior Judgment

• Testbench Code Quality

• Predictor design

• False-pass playbook