This is an archive of the discontinued LLVM Phabricator instance.

NFC: Remove simple_ilist comment mentioning ilist/iplist allocating
ClosedPublic

Authored by lanza on Dec 30 2020, 7:34 PM.

Details

Summary

Allocation was removed from ilist in 2016

Diff Detail

Event Timeline

lanza created this revision.Dec 30 2020, 7:34 PM
lanza requested review of this revision.Dec 30 2020, 7:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 7:34 PM
dexonsmith accepted this revision.Jan 5 2021, 12:35 PM

LGTM. Thanks! (Please reference the commit that removed allocation in the commit message.)

This revision is now accepted and ready to land.Jan 5 2021, 12:35 PM
lanza added a comment.Jan 7 2021, 9:23 PM

On a similar note, I noticed elsewhere after fixing this that you mentioned in a commit message that you intended to remove iplist and ilist and rename as owning_ilist (or something of that sorts). Is that still on the table? Either way, the docs should be updated all around to account for either path name as there are still references to the pre-2016 implementation (e.g. https://llvm.org/docs/ProgrammersManual.html#dss-iplist).

On a similar note, I noticed elsewhere after fixing this that you mentioned in a commit message that you intended to remove iplist and ilist and rename as owning_ilist (or something of that sorts). Is that still on the table?

TBH, a little while after I fixed the UB that was causing miscompiles for us, I was pulled into other things and never got back to this. I'm certainly not working on it right now, although I'm happy to review things in this area (and maybe I'll dive back in a some point if no one else does...).

Either way, the docs should be updated all around to account for either path name as there are still references to the pre-2016 implementation (e.g. https://llvm.org/docs/ProgrammersManual.html#dss-iplist).

Nice catch! If you're interested in writing a patch to fix the docs, that'd be awesome (especially since you seem to have a list of places that need updating), but let me know if not and I'll try to clean it up myself.

TBH, a little while after I fixed the UB that was causing miscompiles for us, I was pulled into other things and never got back to this. I'm certainly not working on it right now, although I'm happy to review things in this area (and maybe I'll dive back in a some point if no one else does...).

Am I recalling correctly that you more or less just wanted to do a wholesale sed 's/\(i\|p\)list/owning_ilist/g (with a more thorough script. of course) across the code base? Or was there more to it?

TBH, a little while after I fixed the UB that was causing miscompiles for us, I was pulled into other things and never got back to this. I'm certainly not working on it right now, although I'm happy to review things in this area (and maybe I'll dive back in a some point if no one else does...).

Am I recalling correctly that you more or less just wanted to do a wholesale sed 's/\(i\|p\)list/owning_ilist/g (with a more thorough script. of course) across the code base? Or was there more to it?

Yes, that sounds right to me; I don't remember if I got around to doing an RFC, and if even I did, it probably deserves a new one (on llvm-dev). ilist is pretty broadly used by out-of-tree code as well as in-tree so it could be could be controversial if the churn isn't justified; IMO, a clear name is better, but an RFC would be good I think.