Skip to content

chore: fix lint#2400

Closed
adr1anh wants to merge 1 commit intonextfrom
adr1anh/fix-nightly
Closed

chore: fix lint#2400
adr1anh wants to merge 1 commit intonextfrom
adr1anh/fix-nightly

Conversation

@adr1anh
Copy link
Contributor

@adr1anh adr1anh commented Dec 1, 2025

This seems to be a bug with clippy since the suggestion triggers a borrow checker error.

Note: we use the new #[expect(clippy::some_warning)] syntax which will trigger a warning in the future if this lint is fixed.

@adr1anh adr1anh requested review from huitseeker and plafer December 1, 2025 18:06
@adr1anh adr1anh added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Dec 1, 2025
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Not a clippy bug. I prefer the fix in 5496f2a

@adr1anh
Copy link
Contributor Author

adr1anh commented Dec 1, 2025

That works too but allocates the vector at every call even if history == None. I’m not sure if that's an issue but I'll let @plafer chime in.

@plafer
Copy link
Contributor

plafer commented Dec 2, 2025

@huitseeker's solution was applied in #2398; though I agree with @adr1anh that we're allocating even when history is off.

Note though that the OverflowTable history should be considered deprecated; it was only used by Process for building a debug trace. Hence at this point I'd probably just create an issue to remove the OverflowTable history when we remove Process (to make sure we don't forget), in which case this issue becomes moot.

@plafer plafer mentioned this pull request Dec 2, 2025
@plafer
Copy link
Contributor

plafer commented Dec 2, 2025

Closing as discussed offline; included a note to remove the OverflowTable history in #2406

@plafer plafer closed this Dec 2, 2025
@huitseeker
Copy link
Contributor

FWIW, the solution I suggested is from #2396 (not #2398) and was not applied to next, since that PR is still open and is generating some discussion — it's going to be a while.

I'm in fact OK with merging this PR, because I would like to prioritize having next fixed quickly, over optimizing allocation in code that we know is deprecated and will be removed shortly.

@plafer would appreciate a stamp if this makes sense to you

@huitseeker huitseeker reopened this Dec 2, 2025
@huitseeker
Copy link
Contributor

Oh, my bad, that was indeed merged 🤦 Don't mind me.

@huitseeker huitseeker closed this Dec 2, 2025
@bobbinth bobbinth deleted the adr1anh/fix-nightly branch December 6, 2025 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants