> On May 19, 2016, 8:48 a.m., Michael Park wrote: > > 3rdparty/stout/include/stout/os/read.hpp, line 118 > > <https://reviews.apache.org/r/47585/diff/1/?file=1387867#file1387867line118> > > > > Let's just wrap this in a `std::unique_ptr<char[]>` instead! > > Benjamin Bannier wrote: > +1, or just a `std::string` since most probably the reflex might be to > *always* use `Owned` instead of `std::unique_ptr`, but that one cannot be > used to manage dynamic arrays. > > Jan Schlicht wrote: > We could also use a `std::vector<char>` or instead, like > ``` > std::vector<char> buffer; > buffer.reserve(BUFSIZ); > ``` > > Jan Schlicht wrote: > Please see the discussion at https://reviews.apache.org/r/47481/ on why > this isn't any of the suggested types. > > Kevin Klues wrote: > Copying here for reference (from https://reviews.apache.org/r/47481/): > > I agree that this needs to be delete[]. Thanks for that. > > I can see why this bug was introduced over time though. If you look back > at revision 1 of this patch, I wasn't using temporary storage at all, which > would have avoided this bug altogether. Unfortunately, this approach was > vetoed by the reviewer, leading to revision 2, which used a stack allocated > array. This also didn't have the bug, but introduced other "stylistic" > problems that the reviewers weren't happy with. In revision 3, I used a > std::vector<char> as you suggest, but it never made it back to reviewboard > because it was vetoed in external feedback. As a compromise, I started using > a unique_ptr<> in revision 4, but this was vetoed as well. Looks like I > overlooked the proper delete to use after 5 revisions through a very simple > patch. > > I would personally do away with the temporary storage and go with the > approach I have in revision 1 on review board. If we are going the temporary > storage route, I would prefer the std::vector<char> approach as you suggest. > > Need to convince a shepherd though.
The decision to use new -> delete semantics instead of unique_ptr<> or vector<char> was for consistency with other code in stout. - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47585/#review133900 ----------------------------------------------------------- On May 19, 2016, 2:49 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47585/ > ----------------------------------------------------------- > > (Updated May 19, 2016, 2:49 p.m.) > > > Review request for mesos, Benjamin Bannier, Bernd Mathiske, and Benjamin > Mahler. > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os/read.hpp > c39140fc17c5b4869c3a90c187ebcb9c284397f4 > > Diff: https://reviews.apache.org/r/47585/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
