Skip to content

Clean up comments after compiler dead-code removal - #8558

Merged
cristianoc merged 2 commits into
masterfrom
agent/cleanup-pr8555-comments
Aug 17, 2026
Merged

Clean up comments after compiler dead-code removal#8558
cristianoc merged 2 commits into
masterfrom
agent/cleanup-pr8555-comments

Conversation

@cristianoc

Copy link
Copy Markdown
Collaborator

Summary

  • Remove comments left dangling after the compiler dead-code cleanup in Remove unused compiler modules and helpers #8555.
  • Restore documentation for the surviving occurrence-counting, initial-environment, and formatter helpers.
  • Correct the peek_slash interface comment.

Why

#8555 legitimately removed the unused compiler code, but a few adjacent comments were either left attached to unrelated definitions or removed even though the documented code remains.

This is documentation-only and intentionally has no changelog entry.

Validation

  • opam exec -- dune build @fmt
  • git diff --check

make checkformat reaches the same OCaml formatter successfully, then cannot run the unrelated ReScript formatting phase in a fresh worktree because the local compiler binary has not been built.

Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
@cristianoc
cristianoc requested a review from cknitt August 17, 2026 08:35
@cristianoc
cristianoc marked this pull request as ready for review August 17, 2026 08:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fb74f7b5d6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread compiler/core/lam_pass_count.ml Outdated
Comment on lines +40 to +41
of uses. A use under a lambda or within a loop is treated as multiple uses,
since neither case is suitable for inlining. The local table [bv] tracks

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Describe captured uses without inflating their count

Correct this description for an identifier referenced once from a lambda or loop: add_one_use records times = 1 with captured = true, rather than treating it as multiple uses, and lam_pass_lets_dce.ml can still inline a captured constant or variable. Documenting the separate captured state is important because maintainers cannot infer the optimizer's inlining decisions from the stated occurrence-count invariant.

Useful? React with 👍 / 👎.

Comment thread compiler/syntax/src/res_doc.mli Outdated
Comment on lines +45 to +50
(** [will_break doc] checks whether [doc] contains forced line breaks.

Forced breaks are not propagated through [customLayout], because doing so
would always select the last layout the algorithm tries. Consumers can use
[will_break] to detect a forced break in a custom layout and explicitly add
[breakParent] to propagate it to the parent document. *)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Limit the will_break contract to the first custom layout

Clarify that this does not generally detect a forced break anywhere in a custom layout. For CustomLayout values whose first alternative has no forced break but a later fallback does, will_break returns false because res_doc.ml only examines CustomLayout (doc :: _); a consumer following this contract may therefore omit the promised break_parent when that fallback is selected.

Useful? React with 👍 / 👎.

@pkg-pr-new

pkg-pr-new Bot commented Aug 17, 2026

Copy link
Copy Markdown

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8558

@rescript/darwin-arm64

npm i https://pkg.pr.new/@rescript/darwin-arm64@8558

@rescript/darwin-x64

npm i https://pkg.pr.new/@rescript/darwin-x64@8558

@rescript/linux-arm64

npm i https://pkg.pr.new/@rescript/linux-arm64@8558

@rescript/linux-x64

npm i https://pkg.pr.new/@rescript/linux-x64@8558

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8558

@rescript/win32-x64

npm i https://pkg.pr.new/@rescript/win32-x64@8558

commit: 6c10ebd

@cknitt cknitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want to address Codex' comments.

Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
@cristianoc
cristianoc enabled auto-merge (rebase) August 17, 2026 08:57
@cristianoc
cristianoc merged commit 0e5d0f0 into master Aug 17, 2026
29 checks passed
@cristianoc
cristianoc deleted the agent/cleanup-pr8555-comments branch August 17, 2026 09:18
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.

2 participants