Skip to content

Fold/visit tweaks#156130

Open
nnethercote wants to merge 2 commits intorust-lang:mainfrom
nnethercote:fold-visit-tweaks
Open

Fold/visit tweaks#156130
nnethercote wants to merge 2 commits intorust-lang:mainfrom
nnethercote:fold-visit-tweaks

Conversation

@nnethercote
Copy link
Copy Markdown
Contributor

Details in individual commits.

r? @WaffleLapkin

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 4, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 4, 2026

WaffleLapkin is not on the review rotation at the moment.
They may take a while to respond.

Copy link
Copy Markdown
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

r=me with nits

Unrelated, you've been assigning me semi-regularly recently, might I ask why? (I don't mind reviewing your PRs, just curious)

View changes since this review

Comment thread compiler/rustc_type_ir/src/visit.rs Outdated
const HAS_RE_ERASED = 1 << 21;

/// Does this have any regions of any kind?
const HAS_REGIONS = TypeFlags::HAS_FREE_REGIONS.bits()
Copy link
Copy Markdown
Member

@WaffleLapkin WaffleLapkin May 4, 2026

Choose a reason for hiding this comment

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

Maybe add has_regions to TypeVisitableExt too and then use it instead of .has_type_flags(TypeFlags::HAS_REGIONS)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure

fn has_type_flags(&self, flags: TypeFlags) -> bool {
let res =
self.visit_with(&mut HasTypeFlagsVisitor { flags }) == ControlFlow::Break(FoundFlags);
res
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.

imo comparing with ControlFlow::Break(FoundFlags) is clearer, because it asserts that Break contains FoundFlags

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Even though FoundFlags is a unit type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that has_vars_bound_at_or_above uses .is_break(). I was fixing the inconsistency and .is_break() seemed nicer.

@nnethercote
Copy link
Copy Markdown
Contributor Author

you've been assigning me semi-regularly recently, might I ask why?

I always choose a reviewer rather than letting bors randomly assign. Usually I choose somebody who knows the relevant part of the code well. Or if it's an easier PR that doesn't require specific expertise I just pick someone who I haven't picked for a while.

- Remove comment about blanket implementation of `FallibleTypeFolder` --
  it does not exist. (It probably used to.)
- Fix some incorrect name references.
- Fix minor grammatical errors.
- Fix stale comment on `visit_region` -- it was a no-op once, but no
  longer.

LLM disclosure: Claude Code identified these problems with comments when
I asked it to review `fold.rs` and `visit.rs`. I verified the
correctness of the problems and made the changes by hand.
- Introduce `HAS_REGIONS`/`has_regions`, which is true if any regions
  are present.
- Add `fold_clauses` to `Shifter` and `RegionFolder` for consistency.
- Simplify `has_type_flags`.
- Remove unnecessary local variables in `HasTypeFlagsVisitor` methods.
- Use `|=` operator in one place.

LLM disclosure: Claude Code suggested these changes when I asked it to
review `fold.rs` and `visit.rs`. I verified the correctness of the
suggestions and made the changes by hand.
@nnethercote nnethercote force-pushed the fold-visit-tweaks branch from fd068fd to dd4ce84 Compare May 5, 2026 00:23
@nnethercote
Copy link
Copy Markdown
Contributor Author

I addressed two of the three comments, and left the is_break unchanged for now pending further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants