Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a LeakSanitizer-reported memory leak in the CRW (CIFF) metadata tree manipulation code by ensuring owned CiffComponent* objects are deleted before being removed from the owning container.
Changes:
- Delete an emptied child directory component before erasing it from
CiffDirectory::components_. - Add braces to make the conditional deletion/erase block explicit and consistent with the existing tag-removal path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@neheb: do you know anything about that failing Cygwin test? Do you think there's a way to fix it? Or should we switch it off on this branch? |
|
It's a new one. I assume it's temporary. @kmilos might know more. |
|
No idea for now... Btw, anyone knows why I can't force push to temporary branches any more? |
Seems to be related to Copilot review rules? Any particular reason for these? Can we have them disabled please? |
Sorry, that was me. I was browsing the repo settings and thought I would try switching on Copilot review. It's implemented as a ruleset and it turns out that it also enabled this force-push setting by default. Do you just want the force-pushes fixed, or do you also not want the Copilot review?
|
|
Ah, thanks. Somewhat questionable/surprising defaults there... There was also "allow branch delete" or some such that prevents mergify (or anyone) to delete a branch after merging. I can turn both of those off. Nothing against reviews, but wouldn't like it to mess with established practices and workflows.... |
Yes, please change anything that is getting in the way. I was looking at the settings from the perspective of tightening up security. I also switched on immutable releases. Something I've been wondering about is whether we could prune the list of people with admin permission. It's quite a long list, many of whom I don't think have been active on this repo for several years, so I wonder if they really need that access. |
|
I'll merge this one despite the failing Cygwin test, because it'll fix the CIFuzz test that's failing on all our PRs. |

This code is causing the CIFuzz test to fail, with this error:
It's because
components_is a vector of raw pointers, and we forgot to calldeleteon the element before erasing it. It's done correctly a few lines below.This bug doesn't exist on the main branch because we switched to using
std::unique_ptrin #3155.