A confusing function

confusing_function.cpp

std::unique_ptr<StoppointOptions::CommandData>
StoppointOptions::CommandData::CreateFromStructuredData(
    const StructuredData::Dictionary &options_dict, Error &error) {
  // Is it just me, or is "data_up" a not very good name?  Could be anything.
  // This is fine, but it is oldschool.  In C++14 we gained std::make_unique
  std::unique_ptr<CommandData> data_up(new CommandData());
  // should be
  // auto data_up = std::make_unique<CommandData>();

  bool found_something = false;
  // so this is concerning because we are derefing a pointer without
  // checking that it actually points somewhere
  bool success = options_dict.GetValueForKeyAsBoolean(
      GetKey(OptionNames::StopOnError), data_up->stop_on_error);

  if (success) {
    found_something = true;
  }

  std::string interpreter_str;
  // we now blow away the value we just set for success, although it is now in found_something
  // so we likely should have just written
  // found_something = success;
  // above
  success = options_dict.GetValueForKeyAsString(
      GetKey(OptionNames::Interpreter), interpreter_str);

  // Now I feel like we should have a more descriptive name than just "success".
  // this var is doing a lot of work.
  if (!success) {
    error.SetErrorString("Missing command language value.");
    return data_up;
  }

  // Call me crazy, but we didn't use the value we set for "found_something" at all,
  // and now we are blowing it away.  Why did we set it before?
  found_something = true;
  auto interp_language = ScriptInterpreter::StringToLanguage(interpreter_str);
  if (interp_language == eScriptLanguageUnknown) {
    error.SetErrorStringWithFormat("Unknown breakpoint command language: %s.",
                                   interpreter_str.c_str());
    return data_up;
  }
  data_up->interpreter = interp_language;
  
  // Now things get stranger.  We need a pointer to an array, which is itself a thin wrapper
  // around a std::vector, which is a wrapper around a narive array?  Why so much indirection?
  StructuredData::Array *user_source;
  success = options_dict.GetValueForKeyAsArray(GetKey(OptionNames::UserSource),
                                               user_source);
  if (success) {
    // This will never change found_something.  It is already true if you are here.
    found_something = true;
    size_t num_elems = user_source->GetSize();
    for (size_t i = 0; i < num_elems; i++) {
      // Bad loop?  Avoid invoking non trivial constructors in loop's scope.
      // This will gain and free dynamic memory once for every pass.
      std::string elem_string;
      // This loop is guarded by if(success) already.
      // Changing the value of success here may make debug confusing.
      // Can we not spare another bool?
      success = user_source->GetItemAtIndexAsString(i, elem_string);
      if (success) {
        data_up->user_source.AppendString(elem_string);
      }
    }
  }
  // Again with the pointless if.  No possible way we get here and have a false
  // in found_something.  
  // This is breaking the point of early return.
  if (found_something) {
    return data_up;
  }
  else {
    return std::unique_ptr<StoppointOptions::CommandData>();
  }
}