Delphi code smells — make the machine find them first
A review pass that starts with an opinion is worth little. A review pass that starts with the compiler and
a leak report is worth a lot, because it is objective and free. Do that half first, every time.
Every warning code, directive and API below was produced by dcc32.exe 37.0 on this machine or copied
from the shipped RTL/DUnitX source, not recalled. Where something could not be verified it says so.
What this skill owns, and what it does not
| Question |
Where it is answered |
Why does this leak? How does ownership work? Free, try/finally, interfaces, refcounting, TObjectList<T>, TComponent, managed records |
delphi, reference/memory.md — not restated here |
What is the right try/except shape? What must I never catch? |
delphi, reference/exceptions.md |
Synchronize vs Queue, locks, FreeOnTerminate |
delphi, reference/concurrency.md |
Who frees the object a DMVC action returns? What is ToFree? |
dmvcframework SKILL.md — not restated here |
| How do I find the leak? How do I prove it? How do I stop it coming back? |
here |
This skill is diagnosis. When a smell needs semantics to explain, it links; it does not re-teach.
Order of attack — an unfamiliar unit
Work down this list. Stop reporting when you run out of items that cost the user something.
- Compile it with warnings and hints on and read every line of output. Free, objective, and the single
highest-yield step.
reference/static-analysis.md.
- Run it with
ReportMemoryLeaksOnShutdown := True. If there is a test suite, run that.
reference/leaks.md.
- Ownership by hand — every
Create in the unit: who frees it, on which paths, including the raising
one. This is where the compiler cannot help you.
- Exception handling — every
except without an on, every empty handler, every try/except that
should have been try/finally.
- Concurrency — anything reachable from a
TThread.Execute or a TTask.Run body.
- Everything else. Naming,
with, long methods, magic numbers.
A leak the compiler cannot see outranks a naming convention, always. If the review output is a wall of
style nits and no lifetime finding, the review was not done. Report defects in the order above, and say for
each one what it costs at runtime — a leak, an AV, a silently wrong result. "This is ugly" is not a finding.
The five-minute pass
# 1. what the compiler already knows (see reference/static-analysis.md for the codes)
dcc32.exe -H -W MyProject.dpr
# 2. the greps that pay for themselves, in rough order of yield
rg -n -U 'try\b[^;]*?\n\s*\w+\s*:=\s*T\w+\.Create' # constructor INSIDE the try -> AV in the finally
rg -n -P '(?s)except\s*(//[^\n]*\n\s*)*end' # swallowed exception
rg -n '\bwith\b\s+\w+.*\bdo\b' # with
rg -n 'FreeAndNil\s*\(\s*[lL]' # FreeAndNil on a local
rg -n 'FreeOnTerminate' # thread lifetime
rg -n '\w+\s*:=\s*\w+\s*\+\s*' # candidate string concat in a loop
Then read reference/smells.md for what each hit means and how to fix it.
Reference files
| File |
Read it when |
reference/static-analysis.md |
Compiler warnings and hints (verified codes), $WARN, turning a warning into an error, the build switches, and an honest note on Pascal Analyzer / SonarDelphi / FixInsight / madExcept / Error Insight |
reference/leaks.md |
Finding, reproducing and fencing a leak: the shipped memory manager, ReportMemoryLeaksOnShutdown, reading the report, RegisterExpectedMemoryLeak, and DUnitX per-test leak detection — the item that makes a leak fail a build |
reference/smells.md |
The catalogue. Each entry: the bad code, what it costs at runtime, the fix, and how to detect it |
The defects worth finding, ranked
Detail and detection for each in reference/smells.md.
| # |
Defect |
Costs you |
| 1 |
Constructor inside the try |
Destroy called on uninitialised memory — AV |
| 2 |
Object created and never freed on some path |
Leak |
| 3 |
Create in a loop, Free after it |
Leak, once per iteration |
| 4 |
Object held as both a class reference and an interface |
Double free, or a dangling pointer |
| 5 |
Two objects holding interfaces to each other |
Leak the shutdown report shows as a matched pair |
| 6 |
except end |
The diagnosis is deleted; the program carries on wrong |
| 7 |
try/except where try/finally was meant |
Resource is released only when something raised |
| 8 |
Result not assigned on every path |
Caller reads garbage. W1035 |
| 9 |
Event handler / observer never unsubscribed |
AV when the publisher fires into a freed subscriber |
| 10 |
A function that returns an object nobody knows they own |
Leak, or a double free |
| 11 |
Worker thread touching the VCL |
AV, once a week, on the customer's machine |
| 12 |
A thread outliving the object it captured |
AV at shutdown |
| 13 |
Method silently hiding an ancestor's virtual |
Wrong method runs. W1010 |
| 14 |
with |
A field added later hijacks a name; silent, compiles |
| 15 |
Magic literal used as a status code |
Wrong branch after any refactor |
Reporting a review
- One line per finding: location, what goes wrong at runtime, the fix. Not an essay.
- Lead with anything that leaks, dangles or returns a wrong value. Style last, or not at all.
- If a compiler warning already covers a finding, cite the code (
W1035) — it turns your opinion into
something the user can put in their build and never hear from you again about.
- Never report a smell you cannot say how to detect. If you cannot name a warning, a tool or a grep for it,
it is a preference, and the user did not ask for your preferences.
1---2name: delphi-code-smells3description: Use when reviewing existing Delphi / Object Pascal code for defects, hunting a memory leak or an access violation, or hardening new code against the classic mistakes. Covers compiler warnings and hints that indicate a real bug, warnings-as-errors, leak detection and how to make a leak fail a build, third-party static analysis, and a catalogue of smells each with a way to find it. Triggers on "code smell", "code review", "review this unit", "refactor this unit", "memory leak", "leak", "leaking", "double free", "access violation", "AV", "static analysis", "lint", "compiler warning", "W1035", "$WARN", "warnings as errors", "FastMM", "ReportMemoryLeaksOnShutdown", "audit this code".4---56# Delphi code smells — make the machine find them first78A review pass that starts with an opinion is worth little. A review pass that starts with the compiler and9a leak report is worth a lot, because it is objective and free. Do that half first, every time.1011Every warning code, directive and API below was **produced by `dcc32.exe` 37.0 on this machine or copied12from the shipped RTL/DUnitX source**, not recalled. Where something could not be verified it says so.1314---1516## What this skill owns, and what it does not1718| Question | Where it is answered |19|----------|---------------------|20| *Why* does this leak? How does ownership work? `Free`, `try/finally`, interfaces, refcounting, `TObjectList<T>`, `TComponent`, managed records | `delphi`, `reference/memory.md` — **not restated here** |21| What is the right `try/except` shape? What must I never catch? | `delphi`, `reference/exceptions.md` |22| `Synchronize` vs `Queue`, locks, `FreeOnTerminate` | `delphi`, `reference/concurrency.md` |23| Who frees the object a DMVC action returns? What is `ToFree`? | `dmvcframework` SKILL.md — **not restated here** |24| **How do I find the leak? How do I prove it? How do I stop it coming back?** | **here** |2526This skill is diagnosis. When a smell needs semantics to explain, it links; it does not re-teach.2728---2930## Order of attack — an unfamiliar unit3132Work down this list. **Stop reporting when you run out of items that cost the user something.**33341. **Compile it with warnings and hints on and read every line of output.** Free, objective, and the single35 highest-yield step. `reference/static-analysis.md`.362. **Run it with `ReportMemoryLeaksOnShutdown := True`.** If there is a test suite, run that.37 `reference/leaks.md`.383. **Ownership by hand** — every `Create` in the unit: who frees it, on which paths, including the raising39 one. This is where the compiler cannot help you.404. **Exception handling** — every `except` without an `on`, every empty handler, every `try/except` that41 should have been `try/finally`.425. **Concurrency** — anything reachable from a `TThread.Execute` or a `TTask.Run` body.436. **Everything else.** Naming, `with`, long methods, magic numbers.4445**A leak the compiler cannot see outranks a naming convention, always.** If the review output is a wall of46style nits and no lifetime finding, the review was not done. Report defects in the order above, and say for47each one what it costs at runtime — a leak, an AV, a silently wrong result. "This is ugly" is not a finding.4849---5051## The five-minute pass5253```bash54# 1. what the compiler already knows (see reference/static-analysis.md for the codes)55dcc32.exe -H -W MyProject.dpr5657# 2. the greps that pay for themselves, in rough order of yield58rg -n -U 'try\b[^;]*?\n\s*\w+\s*:=\s*T\w+\.Create' # constructor INSIDE the try -> AV in the finally59rg -n -P '(?s)except\s*(//[^\n]*\n\s*)*end' # swallowed exception60rg -n '\bwith\b\s+\w+.*\bdo\b' # with61rg -n 'FreeAndNil\s*\(\s*[lL]' # FreeAndNil on a local62rg -n 'FreeOnTerminate' # thread lifetime63rg -n '\w+\s*:=\s*\w+\s*\+\s*' # candidate string concat in a loop64```6566Then read `reference/smells.md` for what each hit means and how to fix it.6768---6970## Reference files7172| File | Read it when |73|------|--------------|74| `reference/static-analysis.md` | Compiler warnings and hints (verified codes), `$WARN`, turning a warning into an error, the build switches, and an honest note on Pascal Analyzer / SonarDelphi / FixInsight / madExcept / Error Insight |75| `reference/leaks.md` | Finding, reproducing and fencing a leak: the shipped memory manager, `ReportMemoryLeaksOnShutdown`, reading the report, `RegisterExpectedMemoryLeak`, and DUnitX per-test leak detection — the item that makes a leak fail a build |76| `reference/smells.md` | The catalogue. Each entry: the bad code, what it costs at runtime, the fix, and how to detect it |7778---7980## The defects worth finding, ranked8182Detail and detection for each in `reference/smells.md`.8384| # | Defect | Costs you |85|---|--------|-----------|86| 1 | Constructor **inside** the `try` | `Destroy` called on uninitialised memory — AV |87| 2 | Object created and never freed on some path | Leak |88| 3 | `Create` in a loop, `Free` after it | Leak, once per iteration |89| 4 | Object held as *both* a class reference and an interface | Double free, or a dangling pointer |90| 5 | Two objects holding interfaces to each other | Leak the shutdown report shows as a matched pair |91| 6 | `except end` | The diagnosis is deleted; the program carries on wrong |92| 7 | `try/except` where `try/finally` was meant | Resource is released only when something raised |93| 8 | `Result` not assigned on every path | Caller reads garbage. **W1035** |94| 9 | Event handler / observer never unsubscribed | AV when the publisher fires into a freed subscriber |95| 10 | A `function` that returns an object nobody knows they own | Leak, or a double free |96| 11 | Worker thread touching the VCL | AV, once a week, on the customer's machine |97| 12 | A thread outliving the object it captured | AV at shutdown |98| 13 | Method silently hiding an ancestor's virtual | Wrong method runs. **W1010** |99| 14 | `with` | A field added later hijacks a name; silent, compiles |100| 15 | Magic literal used as a status code | Wrong branch after any refactor |101102---103104## Reporting a review105106- One line per finding: **location, what goes wrong at runtime, the fix.** Not an essay.107- Lead with anything that leaks, dangles or returns a wrong value. Style last, or not at all.108- If a compiler warning already covers a finding, **cite the code** (`W1035`) — it turns your opinion into109 something the user can put in their build and never hear from you again about.110- Never report a smell you cannot say how to detect. If you cannot name a warning, a tool or a grep for it,111 it is a preference, and the user did not ask for your preferences.
danieleteti