Page MenuHomePhabricator

[llvm-objdump] Add RAII for xar apis
ClosedPublic

Authored by fjricci on Oct 5 2017, 1:55 PM.

Details

Summary

xar_open and xar_iter_new require manual calls to close/free functions
to deallocate resources. This makes it easy to introduce memory leaks,
so add RAII struct wrappers for these resources.

Diff Detail

Repository
rL LLVM

Event Timeline

fjricci created this revision.Oct 5 2017, 1:55 PM
fjricci added a subscriber: sqlbyme.Oct 5 2017, 1:57 PM
fjricci updated this revision to Diff 117885.Oct 5 2017, 1:58 PM

clang-format

davide accepted this revision.Oct 5 2017, 2:00 PM
davide added a subscriber: davide.

LGTM

This revision is now accepted and ready to land.Oct 5 2017, 2:00 PM
compnerd requested changes to this revision.Oct 5 2017, 2:02 PM
compnerd added inline comments.
tools/llvm-objdump/MachODump.cpp
205 ↗(On Diff #117885)

Can you please call this ScopedXarFile?

214 ↗(On Diff #117885)

ScopedXarIter sounds better

This revision now requires changes to proceed.Oct 5 2017, 2:02 PM
fjricci updated this revision to Diff 117889.Oct 5 2017, 2:06 PM
fjricci edited edge metadata.

Xar -> ScopedXar

alexshap added inline comments.
tools/llvm-objdump/MachODump.cpp
205 ↗(On Diff #117885)

this probably should be in the anonymous namespace

fjricci updated this revision to Diff 117895.Oct 5 2017, 2:21 PM

anonymous namespace

alexshap added inline comments.Oct 5 2017, 2:25 PM
tools/llvm-objdump/MachODump.cpp
211 ↗(On Diff #117895)

just to double check - does xar_close work fine with nulls ? (i.e. if xar_open has failed)

fjricci updated this revision to Diff 117928.Oct 5 2017, 4:09 PM

Don't call deallocation routines on null xar objects

compnerd added inline comments.Oct 5 2017, 4:18 PM
tools/llvm-objdump/MachODump.cpp
206 ↗(On Diff #117928)

These really should have the copy and move constructors deleted as well as the assignment operator deleted.

fjricci updated this revision to Diff 117996.Oct 6 2017, 7:38 AM

Delete copy constructor (which implicitly deletes move constructor), and
assignment operator

fjricci updated this revision to Diff 117998.Oct 6 2017, 7:41 AM

clang-format

compnerd accepted this revision.Oct 6 2017, 8:29 AM
This revision is now accepted and ready to land.Oct 6 2017, 8:29 AM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Oct 9 2017, 11:18 AM
llvm/trunk/tools/llvm-objdump/MachODump.cpp
206

I'd consider dropping the "Scoped" suffix & just having XarFile and XarIterator - but up to you.

222

Consider using the init list:

ScopedXarIter() : iter(xar_iter_new()) {}

(or I guess maybe even a NSDMI:

xar_iter_t iter = xar_iter_new();

)

fjricci added inline comments.Oct 9 2017, 1:29 PM
llvm/trunk/tools/llvm-objdump/MachODump.cpp
222

Used initializer list in rL315243. (Not a big fan of the NSDMI because we need to define a destructor anyway)