[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(
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D29892