malloc en t_pokemon -> species en funcion: create_pokemon

varas-c commented 7 years ago

No se si es el lugar indicado o no pero me gustaría comentar algo que vimos con mi grupo.

La funcion en cuestion es la siguiente:

t_pokemon* create_pokemon(t_pkmn_factory* factory, char* species, t_level level) {
  t_pokemon* new_pkmn = NULL;
  t_pokemon* pkmn_types = dictionary_get(factory->pkmn_data, species);

  if(pkmn_types) {
    new_pkmn = malloc(sizeof(t_pokemon));
    new_pkmn->species = species;
    new_pkmn->type = pkmn_types->type;
    new_pkmn->second_type = pkmn_types->second_type;
    new_pkmn->level = level; 
  }
  return new_pkmn;
}

Segun los tests que hay en este repo, el parametro "char* species" recibe un "string literal" (por ejemplo un "bulbasaur" y asigna ese string al campo species del struct new_pkmn. Esto creo que es válido porque un "string literal" puede "vivir" fuera del scope de la función porque es alojado en una zona de solo lectura (aalgo asi). Por lo que al retornar el struct new_pkmn ese campo "species" no va a ser "pisado", tiene digamos "memoria reservada" (no estoy muy seguro de esto, si me pueden corregir, buenisimo)

La cuestion es que nosotros suponiamos que, si la funcion "create pokemon" me reserva memoria para crear un pokemon, nos debería reservar memoria para su nombre (es un campo del struct, asi que tiene sentido), cosa que no sucede

El problema que tuvimos es que usabamos esta funcion para crear los pokemon PERO pasabamos como parametro al char* species un puntero (reservado con malloc) con el nombre del pokemon que queríamos crear. Despues, borrabamos ese puntero que usabamos como parametro pero... como la funcion create_pokemon lo unico que hace es asignar nuestro "puntero parametro" al campo "species" perdíamos tambien el campo species, por lo que nos quedaba un pokemon con el nombre "corrupto".

Estuvimos un rato bastante largo para encontrar este error. Al menos yo agregaría el siguiente cambio a la funcion:

new_pkmn->species = species; //actual

new_pkmn->species = strdup(species); //nuevo

No se si está bien o mal lo que propongo, pero fue un problema que nos costó encontrar y capaz a alguien mas le pasó y leyendo esto capaz lo puede solucionar.

Si me pueden responder, les agradecería. Gracias! :D

tferraro commented 7 years ago

Buenas!

La biblioteca para crear los pokémons estaba pensada como una herramienta auxiliar al uso del tp, utilizando el string que tengan referenciados desde otra estructura de trabajo, de ahí que no trabaja con una copia, ni tampoco elimina la memoria utilizada en el nombre, y lo usa solo a modo de transito nomás.

El hecho que en las pruebas se utilice un literal fue, capas, contraproducente para mostrar esto, por la necesidad de mantenerlas simples.

Sin embargo, estaría bueno como feature sobre create_pokemon y destroy_pkmn_factory que trabajaran con un duplicado de lo que se le pase. Evalúo la posibilidad de ampliarlo y el impacto que pueda tener en otros tps y te comento. Probablemente esto termine en una siguiente versión con compatiblidad a la anterior (teniendo en cuenta que estamos a 2 sábados de la entrega).

Saludos,

Tom

varas-c commented 7 years ago

Entendido! Si, falta poco para la entrega y no sería bueno cambiar a esta altura. Solo era una duda y una nota sobre la función.

Gracias :)

tferraro commented 7 years ago

No lo cierres aún :wink:, el feature siempre sirve!

sisoputnfrba / so-pkmn-utils

malloc en t_pokemon -> species en funcion: create_pokemon #3