[PATCH] D29892: ar: add llvm-dlltool support

[Martell Malone via Phabricator via llvm-commits]

martell added inline comments.


================
Comment at: lib/Object/COFFImportFile.cpp:584-585
+    if (E.isWeak()) {
+      std::vector<uint8_t> *WeakBuffer1 = new std::vector<uint8_t>();
+      std::vector<uint8_t> *WeakBuffer2 = new std::vector<uint8_t>();
+      Members.push_back(
----------------
ruiu wrote:
> ruiu wrote:
> > martell wrote:
> > > ruiu wrote:
> > > > Please don't use raw `new`s. You really want to avoid doing that, not only in this patch, but in other patches as well. You are leaking memory. I believe you are doing this because otherwise you would be accessing free'd memory, but leaking memory is also a bug which is not acceptable. Using `new` is not a solution for a use-after-free bug.
> > > Sorry about that Rui, I didn't even realise I used `new` here.
> > > This doesn't even need `new` because it is pushed back onto the vector.
> > > Updating the patch now. Also fixed and tested with mingw-w64.
> > Why do you have to pass buffers in the first place? Their lifetime is till the end of this block, so it seems unnecessary.
> Can you address this comment?
In general I am just following the style of the rest of the function.

std::vector<uint8_t> ImportDescriptor; -> createImportDescriptor
std::vector<uint8_t> NullImportDescriptor; -> createNullImportDescriptor
std::vector<uint8_t> NullThunk; -> createNullThunk

std::vector<uint8_t> WeakBuffer1;
std::vector<uint8_t> WeakBuffer2;

Maybe a suggestion of a better name?
Unless you want a change to the signature of createWeakExternal?


Repository:
  rL LLVM

https://reviews.llvm.org/D29892