[llvm] r216362 - ExecutionEngine: unique_ptr-ify

[David Blaikie]

On Sun, Aug 24, 2014 at 5:58 PM, Dylan Noblesmith <nobled at dreamwidth.org> wrote:
> Author: nobled
> Date: Sun Aug 24 19:58:18 2014
> New Revision: 216362
>
> URL: http://llvm.org/viewvc/llvm-project?rev=216362&view=rev
> Log:
> ExecutionEngine: unique_ptr-ify
>
> NFC.
>
> Modified:
>     llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
>
> Modified: llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp?rev=216362&r1=216361&r2=216362&view=diff
> ==============================================================================
> --- llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp (original)
> +++ llvm/trunk/lib/ExecutionEngine/ExecutionEngine.cpp Sun Aug 24 19:58:18 2014
> @@ -256,19 +256,9 @@ const GlobalValue *ExecutionEngine::getG
>
>  namespace {
>  class ArgvArray {
> -  char *Array;
> -  std::vector<char*> Values;
> +  std::unique_ptr<char[]> Array;
> +  std::vector<std::unique_ptr<char[]>> Values;
>  public:
> -  ArgvArray() : Array(nullptr) {}
> -  ~ArgvArray() { clear(); }
> -  void clear() {
> -    delete[] Array;
> -    Array = nullptr;
> -    for (size_t I = 0, E = Values.size(); I != E; ++I) {
> -      delete[] Values[I];
> -    }
> -    Values.clear();
> -  }
>    /// Turn a vector of strings into a nice argv style array of pointers to null
>    /// terminated strings.
>    void *reset(LLVMContext &C, ExecutionEngine *EE,
> @@ -277,32 +267,34 @@ public:
>  }  // anonymous namespace
>  void *ArgvArray::reset(LLVMContext &C, ExecutionEngine *EE,
>                         const std::vector<std::string> &InputArgv) {
> -  clear();  // Free the old contents.
> +  Values.clear();  // Free the old contents.
> +  Values.reserve(InputArgv.size());

Adding the reserve here looks like a somewhat more interesting
semantic change than the rest of this cleanup - it would be good to
commit that separately rather than alongside the rest of this cleanup.

(reserve can have weird/negative effects in some cases, though I doubt
they arise here - it still gives me pause when it's added, to make
sure it's not one of the bad cases)

>    unsigned PtrSize = EE->getDataLayout()->getPointerSize();
> -  Array = new char[(InputArgv.size()+1)*PtrSize];
> +  Array = make_unique<char[]>((InputArgv.size()+1)*PtrSize);
>
> -  DEBUG(dbgs() << "JIT: ARGV = " << (void*)Array << "\n");
> +  DEBUG(dbgs() << "JIT: ARGV = " << (void*)Array.get() << "\n");
>    Type *SBytePtr = Type::getInt8PtrTy(C);
>
>    for (unsigned i = 0; i != InputArgv.size(); ++i) {
>      unsigned Size = InputArgv[i].size()+1;
> -    char *Dest = new char[Size];
> -    Values.push_back(Dest);
> +    auto DestOwner = make_unique<char[]>(Size);
> +    char *Dest = DestOwner.get();
> +    Values.push_back(std::move(DestOwner));

Could this line be moved to the end of the loop (and then the rest of
the loop can just use DestOwner (renamed back to Dest) directly,
rather than having to have the extra "Dest" variable)?

And/or possibly the body of the loop factored out into a function that
returns the unique_ptr<char[]> so the loop body is just one line:
Values.push_back(DoTheThing(...));

Not sure if that's the right refactoring - perhaps the loop body has
too many variables to access or doesn't have a nice logical name, etc.

>      DEBUG(dbgs() << "JIT: ARGV[" << i << "] = " << (void*)Dest << "\n");
>
>      std::copy(InputArgv[i].begin(), InputArgv[i].end(), Dest);
>      Dest[Size-1] = 0;

Unrelated, but it'd be nice if this was "= '\0'" since it seems this
is a null terminated character sequence, rather than just raw bytes.

>      // Endian safe: Array[i] = (PointerTy)Dest;
> -    EE->StoreValueToMemory(PTOGV(Dest), (GenericValue*)(Array+i*PtrSize),
> +    EE->StoreValueToMemory(PTOGV(Dest), (GenericValue*)(&Array[i*PtrSize]),
>                             SBytePtr);
>    }
>
>    // Null terminate it
>    EE->StoreValueToMemory(PTOGV(nullptr),
> -                         (GenericValue*)(Array+InputArgv.size()*PtrSize),
> +                         (GenericValue*)(&Array[InputArgv.size()*PtrSize]),
>                           SBytePtr);
> -  return Array;
> +  return Array.get();
>  }
>
>  void ExecutionEngine::runStaticConstructorsDestructors(Module &module,
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits