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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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(
----------------
martell wrote:
> martell wrote:
> > 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?
> I would assume the others use std::vector<uint8_t> in the case that someone wants to use this function on a premade buffer of their own. Possible Public API???
> I am happy to put the buffer inside the createWeakExternal function itself if you want?
I don't think it's about style. Why do you have to pass WeakBuffer1 and WeakBuffer2? If there's no need, please don't do that.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892