Skip to content

feat(decompiler): Rec 31 #31-3 Stage 2C-min — xml.y content-Reference tmp to stack-local#77

Merged
CryptoJones merged 1 commit into
masterfrom
feat/rec-31-stage2c-min-xml-tmp-stack-local
May 26, 2026
Merged

feat(decompiler): Rec 31 #31-3 Stage 2C-min — xml.y content-Reference tmp to stack-local#77
CryptoJones merged 1 commit into
masterfrom
feat/rec-31-stage2c-min-xml-tmp-stack-local

Conversation

@CryptoJones

Copy link
Copy Markdown
Owner

Stage 2C-min step 1 from docs/decompiler/RAII_STAGE_2C_XML.md.

The semantic action for the content Reference rule allocated a temporary string on the heap, appended a single int4 to it, passed it by reference to print_content, then immediately deleted it:

| content Reference {
    string *tmp = new string();
    *tmp += $2;
    print_content(*tmp);
    delete tmp;
    global_scan->setmode(XmlScan::CharDataMode);
  }

The temporary never escapes its enclosing block — pure code-smell. Replacement is a stack-local:

| content Reference {
    string tmp;
    tmp += $2;
    print_content(tmp);
    global_scan->setmode(XmlScan::CharDataMode);
  }

print_content takes const string &, so passing the stack-local by reference is identical to dereferencing the previous raw owner. No bison regeneration needed: the change is inside a semantic action body (which bison copies verbatim into yyparse's switch); xml.y and xml.cc:1790 get the same hand-edit and stay in sync.

Does NOT touch the five other bison-mediated raw-new sites (xml.y:150, 153, 198, 200 and the parallel sites in xml.cc) — those need the bison %union%define api.value.type variant migration scoped in the design doc, which is a strategic sprint of its own.

Does NOT add xml.y / xml.cc to cppRaiiAudit's PROTECTED_FILES yet — would require enhancing the audit task to accept per-file line-range exclusions for the five bison-mediated sites. Tracked in the design doc as deferred.

CHANGELOG.md [Unreleased] section updated to record Stage 2C-min shipped, per the per-PR changelog rule.

Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/

… tmp to stack-local

Stage 2C-min step 1 from docs/decompiler/RAII_STAGE_2C_XML.md.

The semantic action for the `content Reference` rule allocated a
temporary string on the heap, appended a single int4 to it, passed
it by reference to `print_content`, then immediately deleted it:

  | content Reference {
      string *tmp = new string();
      *tmp += $2;
      print_content(*tmp);
      delete tmp;
      global_scan->setmode(XmlScan::CharDataMode);
    }

The temporary never escapes its enclosing block — pure code-smell.
Replacement is a stack-local:

  | content Reference {
      string tmp;
      tmp += $2;
      print_content(tmp);
      global_scan->setmode(XmlScan::CharDataMode);
    }

`print_content` takes `const string &`, so passing the stack-local
by reference is identical to dereferencing the previous raw owner.
No bison regeneration needed: the change is inside a semantic action
body (which bison copies verbatim into yyparse's switch); xml.y
and xml.cc:1790 get the same hand-edit and stay in sync.

Does NOT touch the five other bison-mediated raw-new sites
(xml.y:150, 153, 198, 200 and the parallel sites in xml.cc) — those
need the bison %union %define api.value.type variant migration
scoped in the design doc, which is a strategic sprint of its own.

Does NOT add xml.y / xml.cc to cppRaiiAudit's PROTECTED_FILES yet
— would require enhancing the audit task to accept per-file
line-range exclusions for the five bison-mediated sites. Tracked
in the design doc as deferred.

CHANGELOG.md [Unreleased] section updated to record Stage 2C-min
shipped, per the per-PR changelog rule
(feedback_changelog_per_pr.md).

Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@CryptoJones
CryptoJones merged commit de5789e into master May 26, 2026
16 checks passed
@CryptoJones
CryptoJones deleted the feat/rec-31-stage2c-min-xml-tmp-stack-local branch May 26, 2026 21:46
CryptoJones added a commit that referenced this pull request May 26, 2026
…wns children via unique_ptr

Stage 2C step 2 from docs/decompiler/RAII_STAGE_2C_XML.md.

Element's `children` vector now owns its members as `unique_ptr<Element>`
instead of raw `Element *`. The classic "destructor walks the vector
and manually deletes each entry" pattern is replaced by the compiler-
generated default destructor + the vector's destructor + the unique_ptr's
destructor.

Header (xml.hh):

  typedef vector<Element *> List;
  // becomes:
  typedef vector<unique_ptr<Element>> List;

  void addChild(Element *child) { children.push_back(child); }
  // becomes:
  void addChild(unique_ptr<Element> child) { children.push_back(move(child)); }

  Element *getRoot(void) const { return *children.begin(); }
  // becomes:
  Element *getRoot(void) const { return children.front().get(); }

Caller (xml.y / xml.cc TreeHandler::startElement):

  Element *newel = new Element(cur);
  cur->addChild(newel);
  cur = newel;
  // becomes:
  auto owned = make_unique<Element>(cur);
  Element *newel = owned.get();   // raw observer for cur + the rest
  cur->addChild(move(owned));     // parent takes ownership
  cur = newel;

Destructor (xml.y / xml.cc Element::~Element):

  Element::~Element(void) {
    List::iterator iter;
    for(iter=children.begin();iter!=children.end();++iter)
      delete *iter;
  }
  // becomes:
  Element::~Element(void) = default;

The hand-edit-parallel technique keeps xml.y and xml.cc in lockstep
without bison regeneration (both modified files are outside the
bison-generated table-driven yyparse body).

Consumer-side: 6 files (bfd_arch.cc, marshal.cc, raw_arch.cc,
slgh_compile.cc, testfunction.cc, xml_arch.cc) updated to use
iter->get() instead of *iter when extracting raw Element * from a
List::const_iterator. `(*iter)->method()` patterns are unchanged
(unique_ptr's operator-> still works); only the raw-pointer-extract
sites needed the .get() addition.

What does NOT change in this PR:
  - The bison %union of raw pointers (string*, Attributes*, NameValue*).
    Those need the Option A C++-variant migration per the design doc.
  - The Document return-value migration from xml_tree(). That's
    Stage 2C step 3 — separate PR.
  - cppRaiiAudit's PROTECTED_FILES does NOT yet include xml.{y,cc};
    the bison %union sites would still trigger it. Same exception-
    handling work deferred from #77.

Behavior preserved: Element ownership lifecycle is identical to
before — the parent's destructor still cleans up each child; the
only differences are (a) cleanup is automatic instead of manual,
and (b) Element is now move-only (was implicitly copy-constructible
via shallow raw pointers, but no code actually copied Elements
by value so this is invisible at the call sites).

Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CryptoJones added a commit that referenced this pull request May 26, 2026
…wns children via unique_ptr (#78)

Stage 2C step 2 from docs/decompiler/RAII_STAGE_2C_XML.md.

Element's `children` vector now owns its members as `unique_ptr<Element>`
instead of raw `Element *`. The classic "destructor walks the vector
and manually deletes each entry" pattern is replaced by the compiler-
generated default destructor + the vector's destructor + the unique_ptr's
destructor.

Header (xml.hh):

  typedef vector<Element *> List;
  // becomes:
  typedef vector<unique_ptr<Element>> List;

  void addChild(Element *child) { children.push_back(child); }
  // becomes:
  void addChild(unique_ptr<Element> child) { children.push_back(move(child)); }

  Element *getRoot(void) const { return *children.begin(); }
  // becomes:
  Element *getRoot(void) const { return children.front().get(); }

Caller (xml.y / xml.cc TreeHandler::startElement):

  Element *newel = new Element(cur);
  cur->addChild(newel);
  cur = newel;
  // becomes:
  auto owned = make_unique<Element>(cur);
  Element *newel = owned.get();   // raw observer for cur + the rest
  cur->addChild(move(owned));     // parent takes ownership
  cur = newel;

Destructor (xml.y / xml.cc Element::~Element):

  Element::~Element(void) {
    List::iterator iter;
    for(iter=children.begin();iter!=children.end();++iter)
      delete *iter;
  }
  // becomes:
  Element::~Element(void) = default;

The hand-edit-parallel technique keeps xml.y and xml.cc in lockstep
without bison regeneration (both modified files are outside the
bison-generated table-driven yyparse body).

Consumer-side: 6 files (bfd_arch.cc, marshal.cc, raw_arch.cc,
slgh_compile.cc, testfunction.cc, xml_arch.cc) updated to use
iter->get() instead of *iter when extracting raw Element * from a
List::const_iterator. `(*iter)->method()` patterns are unchanged
(unique_ptr's operator-> still works); only the raw-pointer-extract
sites needed the .get() addition.

What does NOT change in this PR:
  - The bison %union of raw pointers (string*, Attributes*, NameValue*).
    Those need the Option A C++-variant migration per the design doc.
  - The Document return-value migration from xml_tree(). That's
    Stage 2C step 3 — separate PR.
  - cppRaiiAudit's PROTECTED_FILES does NOT yet include xml.{y,cc};
    the bison %union sites would still trigger it. Same exception-
    handling work deferred from #77.

Behavior preserved: Element ownership lifecycle is identical to
before — the parent's destructor still cleans up each child; the
only differences are (a) cleanup is automatic instead of manual,
and (b) Element is now move-only (was implicitly copy-constructible
via shallow raw pointers, but no code actually copied Elements
by value so this is invisible at the call sites).

Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CryptoJones added a commit that referenced this pull request May 27, 2026
… to cppRaiiAudit (#87)

Closes the carried "audit-gate add" deferral from Stage 2C-min (PR #77).
After #46 / #51 / #73 / #77 / #78 / #82, the xml.y and xml.cc epilogue
sections are raw-`new`-free; only the four bison `%union` semantic-action
sites remain, and those block on the Option A variant-mode rewrite (its own
strategic sprint per docs/decompiler/RAII_STAGE_2C_XML.md).

`cppRaiiAudit.gradle`'s `PROTECTED_FILES` migrated from `Set<String>` to
`Map<String, List<List<Integer>>>` so per-file `[startLine, endLine]`
exclusion ranges can be expressed. The audit's per-line loop checks each
file's exclusion list before applying the raw-`new` regex. Existing
foundation files (address.cc / space.cc / rangeutil.cc / marshal.cc /
marshal.hh) carry empty exclusion lists — every line is audited.

`xml.y` exclusions: lines 150, 153, 198, 200 (the four `$$ = new ...`
semantic actions in `attsinglemid`, `attdoublemid`, `stagstart`,
`SAttribute`). `xml.cc` exclusions: lines 1598, 1616, 1736, 1748 — the
parallel bison-generated `case N:` blocks inside `yyparse`'s switch.
Line numbers are stable against the bison-3.0.4 output committed in tree.

Stage 2C design doc + CHANGELOG `[Unreleased]` + SprintPlanning Sprint 6
all updated to reflect the audit-gate add as shipped.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CryptoJones added a commit that referenced this pull request May 27, 2026
Sprint 10 is code-closed — only Aaron-UI-action items
(Automatic Dependency Submission workflow, immutable-releases
toggle) and the deferred Stage 3 step 6 `-Werror` + ErrorProne
ratchet (its own sprint) remain. Cut v26.1.11 to ship the
[Unreleased] backlog accumulated since v26.1.10:

- Rec 31 RAII Stage 2A complete (#46 marshal.cc buffer ownership)
- Rec 31 RAII Stage 2B complete (#51, #73 xml.cc lvalue +
  global_scan lifetime)
- Rec 31 RAII Stage 2C complete (#77 stack-local, #78 Element
  parse-tree, #82 Document return-value, #87 audit-gate)
- Rec 31 RAII Stage 3 first migrations (#89 cover.cc gate,
  #90 comment.cc migration + gate)
- Rec 28 closeout — `ignoreAudit` Stage 2 strict-by-default (#43)
- Rec 13/14 OSS-Fuzz upstream submission attempted + rejected;
  wrapper deleted, harnesses retained for in-tree fuzzing
  (google/oss-fuzz#15545, #48, #49, #84)
- Rec 20 RMI VMARG removed from launch.properties (#81)
- release.yml Windows zip glob fix (#88) — `_windows_*` →
  `_win_*` so `getCurrentPlatformName()`'s `win_x86_64` token
  matches. Unblocks the matrix's `publish_release` job that's
  been silently skipped since v26.1.6.

**v26.1.11 is the first release expected to actually appear on
the public Releases page.** v26.1.6/8/9/10 are backlogged in
drafts under "Immutable Releases" tag lockout; not recoverable
under their original tags.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant