This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Properly destruct allocated Records
ClosedPublic

Authored by tbaeder on Sep 16 2022, 10:01 AM.

Details

Summary

As discussed with @aaron.ballman on IRC.

This reverts https://reviews.llvm.org/rGa8843643cd75d0e93ebcf3f30b470d2b8e59868d

I have ASan enabled locally and with this patch, the test suite now works fine.

Diff Detail

Event Timeline

tbaeder created this revision.Sep 16 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 10:01 AM
tbaeder requested review of this revision.Sep 16 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 10:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Other than the pedantry above, LGTM.

clang/lib/AST/Interp/Program.h
48

I SUSPECT that by a reading of our rules here, we aren't allowed to use auto here. That said, I'm sure I don't want to have you spell out the whole LHS.

THOUGH the name "It" is a little misleading, as this is NOT an iterator, it ends up being a std::pair or something I think...

tbaeder added inline comments.Sep 16 2022, 10:30 AM
clang/lib/AST/Interp/Program.h
48

Would RecordPair be better?

erichkeane accepted this revision.Sep 16 2022, 10:33 AM
erichkeane added inline comments.
clang/lib/AST/Interp/Program.h
48

Sure, good enough.

Feel free to come up with whatever and commit.

This revision is now accepted and ready to land.Sep 16 2022, 10:33 AM
tbaeder marked 2 inline comments as done.Sep 17 2022, 1:37 AM
aaron.ballman accepted this revision.Sep 21 2022, 7:18 AM

LGTM as well (modulo Erich's comments).

This revision was automatically updated to reflect the committed changes.