[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

courgette/assembly_program.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "courgette/assembly_program.h"
 
 #include <memory.h>
 #include <stddef.h>
 #include <stdint.h>
+
 #include <algorithm>
 #include <map>
 #include <set>
 #include <sstream>
+#include <utility>

[grt (UTC plus 2), 2016/01/26 16:39:54]

the four includes above this appear to be unused, and utility isn't needed as
per my comment at the end of the file

[huangs, 2016/01/26 18:42:24]

Done.

 #include <vector>
 
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/scoped_ptr.h"
-
 #include "courgette/courgette.h"
 #include "courgette/encoded_program.h"
 
 namespace courgette {
 
 namespace {
 
 // Sets the current address for the emitting instructions.
 class OriginInstruction : public Instruction {
  public:
@@ skipping 328 matching lines @@
       ++index;
       ++fill_infill_count;
     }
   }
 
   VLOG(1) << "  fill forward " << fill_forward_count
           << "  backward " << fill_backward_count
           << "  infill " << fill_infill_count;
 }
 
-EncodedProgram* AssemblyProgram::Encode() const {
+scoped_ptr<EncodedProgram> AssemblyProgram::Encode() const {
   scoped_ptr<EncodedProgram> encoded(new EncodedProgram());
 
   encoded->set_image_base(image_base_);
 
   if (!encoded->DefineLabels(abs32_labels_, rel32_labels_))
-    return NULL;
+    return nullptr;
 
   for (size_t i = 0;  i < instructions_.size();  ++i) {
     Instruction* instruction = instructions_[i];
 
     switch (instruction->op()) {
       case ORIGIN: {
         OriginInstruction* org = static_cast<OriginInstruction*>(instruction);
         if (!encoded->AddOrigin(org->origin_rva()))
-          return NULL;
+          return nullptr;
         break;
       }
       case DEFBYTE: {
         uint8_t b = static_cast<ByteInstruction*>(instruction)->byte_value();
         if (!encoded->AddCopy(1, &b))
-          return NULL;
+          return nullptr;
         break;
       }
       case DEFBYTES: {
         const uint8_t* byte_values =
             static_cast<BytesInstruction*>(instruction)->byte_values();
         size_t len = static_cast<BytesInstruction*>(instruction)->len();
 
         if (!encoded->AddCopy(len, byte_values))
-          return NULL;
+          return nullptr;
         break;
       }
       case REL32: {
         Label* label = static_cast<InstructionWithLabel*>(instruction)->label();
         if (!encoded->AddRel32(label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case REL32ARM: {
         Label* label =
             static_cast<InstructionWithLabelARM*>(instruction)->label();
         uint16_t compressed_op =
             static_cast<InstructionWithLabelARM*>(instruction)->compressed_op();
         if (!encoded->AddRel32ARM(compressed_op, label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case ABS32: {
         Label* label = static_cast<InstructionWithLabel*>(instruction)->label();
         if (!encoded->AddAbs32(label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case ABS64: {
         Label* label = static_cast<InstructionWithLabel*>(instruction)->label();
         if (!encoded->AddAbs64(label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case MAKEPERELOCS: {
         if (!encoded->AddPeMakeRelocs(kind_))
-          return NULL;
+          return nullptr;
         break;
       }
       case MAKEELFRELOCS: {
         if (!encoded->AddElfMakeRelocs())
-          return NULL;
+          return nullptr;
         break;
       }
       case MAKEELFARMRELOCS: {
         if (!encoded->AddElfARMMakeRelocs())
-          return NULL;
+          return nullptr;
         break;
       }
       default: {
         NOTREACHED() << "Unknown Insn OP kind";
       }
     }
   }
 
-  return encoded.release();
+  return encoded;
 }
 
 Instruction* AssemblyProgram::GetByteInstruction(uint8_t byte) {
   if (!byte_instruction_cache_) {
     Instruction** ram = nullptr;
     if (!base::UncheckedMalloc(sizeof(Instruction*) * 256,
                                reinterpret_cast<void**>(&ram))) {
       return nullptr;
     }
     byte_instruction_cache_.reset(ram);
@@ skipping 63 matching lines @@
     } else {
       ++it;
     }
   }
 
   return true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
-Status Encode(AssemblyProgram* program, EncodedProgram** output) {
-  *output = NULL;
-  EncodedProgram *encoded = program->Encode();
-  if (encoded) {
-    *output = encoded;
-    return C_OK;
-  } else {
+Status Encode(const AssemblyProgram& program,

[grt (UTC plus 2), 2016/01/26 16:39:54]

i don't see the point of this function since the returned Status doesn't provide
any information beyond the non-nullness of |output|. can all callers of this be
changed to:
  scoped_ptr<EncodedProgram> encoding(program->Encode());
  if (!encoding) {
    // OMG! C_GENERAL_ERROR!
  } else {
    do stuff with |encoding|.
  }

[huangs, 2016/01/26 18:42:24]

I think the intention of these weird wrappers is to allow callers to just
recognize C_OK, and pass whatever error along without needing to know details? 
Also, note that Problem() actually calls exit(0).

+              scoped_ptr<EncodedProgram>* output) {
+  output->reset(nullptr);

[grt (UTC plus 2), 2016/01/26 16:39:54]

please document this subtlety
  // Explicitly release any memory associated with the output before encoding.

[huangs, 2016/01/26 18:42:24]

Done.

+
+  scoped_ptr<EncodedProgram> encoded = program.Encode();

[grt (UTC plus 2), 2016/01/26 16:39:54]

this intermediate variable isn't needed:
  *output = program.Encode();
  return (*output) ? C_OK : C_GENERAL_ERROR;

[huangs, 2016/01/26 18:42:24]

Done.

+  if (!encoded)
     return C_GENERAL_ERROR;
-  }
+
+  *output = std::move(encoded);
+  return C_OK;
 }
 
 }  // namespace courgette