> On Nov. 2, 2016, 5:39 p.m., Benjamin Bannier wrote: > > src/jvm/jvm.cpp, lines 97-102 > > <https://reviews.apache.org/r/53386/diff/1/?file=1551496#file1551496line97> > > > > This will work as long nobody adds new `return` statements before the > > corresponding `delete`. > > > > Could we use a `std::vector<JavaVMOption>` here for easier maintenance? > > Note that you can access the raw memory block of the entries like > > > > // conditional on `options.size() != 0` > > vmArgs.options = &opts[0]; > > Neil Conway wrote: > It is a bit annoying to maintain, although that is true for any situation > in which we're using `new`/`delete`, which unfortunately is quite common > throughout the code base. > > Personally I think `unique_ptr<JavaVMOption>` would be the best choice (I > believe we can't use `Owned` since it doesn't have the right specialization > to use `delete[]` when necessary). Using `vector` here seems a bit > unfortunate, since the array is a fixed size; I also feel a bit squimish > about accessing the underlying raw memory block.
Yes, a `std::unique_ptr<JavaVMOption[]>` would be a perfect fit here, but I believe its usage is usually discouraged even though our own `Owned<T>` does not support wrapping arrays. In any case the memory would still be allocated on the heap like for a `vector` (and we don't use `std::experimental::dynarray` either). Re:`vector`, above pattern is guaranteed to be safe in C++11 and actually pretty idomatic for working with C APIs. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53386/#review154578 ----------------------------------------------------------- On Nov. 2, 2016, 5:23 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53386/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2016, 5:23 p.m.) > > > Review request for mesos and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > Fixed memory leak in JVM code. > > > Diffs > ----- > > src/jvm/jvm.cpp 62f7857180162c510269b5d10f5add94ab354fa2 > > Diff: https://reviews.apache.org/r/53386/diff/ > > > Testing > ------- > > `make check` > > Verified that observed `clang-tidy` leak warning goes away with this change. > > > Thanks, > > Neil Conway > >
