genbetadev / Genbeta-Dev-Engine

Desarrollo de un Game Engine básico sobre C++ y SFML 2.1
MIT License
63 stars 32 forks source link

Nuevo sistema de Logs con streams y operador << #67

Closed InExtremaRes closed 10 years ago

InExtremaRes commented 10 years ago

Tal como se discutió en el issue #20, les presento mi propuesta para la clase Logs, usando streams y el operador <<.

Si bien aun no está totalmente implementada, ya es totalmente funcional y se puede apreciar claramente la idea.

EDIT (17 nov 2013): Con los comentarios aquí expuestos he creado un pull request #70 con todos los cambios realizados. La rama y los archivos a los que antes mencionaba este mensaje ya no existen.

Saludos y abrazos a todos.

adrigm commented 10 years ago

Buenísimo @InExtremaRes, para mí mucho mejor y más estilo C++. Me ha gustado en especial el detalle de añadir facilmente salida para los objetos del engine sobrecargando el operador <<. Si nos molestamos en implementarlo escribiremos buenos logs como churros.

Por cierto, de mayor quiero documentar como tú! ;)

adrigm commented 10 years ago

Una duda que me viene a la cabeza, que la verdad hasta ahora no había caído nunca: Cuando se distribuya el engine para el usuario donde solo usará los archivos headers y las correspondientes bibliotecas generadas, las macros __FILE__ y __LINE__ actúan sin problemas?

InExtremaRes commented 10 years ago

Muchas gracias @adrigm :)

Respecto a __LINE__ y __FILE__,, a mi entender y tal como dices, son macros y como tales son reemplazadas por constantes por el preprocesador, así que quedan como datos insertados en los binario, dentro de la biblioteca. El usuario final debería verlos sin problemas aun en modo Release.

Lo malo, es que según como sea compilada la librería, el compilador podría colocar la ruta absoluta completa del archivo, tal como era al momento de la compilación. Es decir, si yo compilo la librería y tu la usas, tu verías la expansión de __FILE__ como: /home/inextremares/GDE/.../Log.cpp, por decir algo. Para solucionarlo, se podría hacer un strip de la ruta y mantener solo el nombre.

adrigm commented 10 years ago

@InExtremaRes, y de que forma se pueda hacer para actuar justo después del preprocesador y justo antes de la compilación. que sería el momento de truncar las rutas y convertirlas en relativas.

ficion commented 10 years ago

En GCC hay un macro llamado __BASE_FILE__, que expande al nombre del fichero en cuestión (sin la ruta). Pero dado que otros usan otros compiladores, ésto no sirve mucho.

No sé si se puede hacer algo como lo que tú dices @adrigm, porque para cortarlas hay que usar algo como una función y eso queda fuera del prepocesador. Por otro lado, podríamos redefinir algún macro especial en cada archivo fuente, pero no sé si es una buena opción.

No estoy seguro, pero CMake parece que permite declarar macros en archivos, no sé hasta qué punto puede eso ayudar, pero la respuesta podría estar ahí.

Por último, hay otro macro que puede ser útil, que es __func__, desde C99; pero no tengo ni la más remota idea si funciona en C++, ni en otro compilador que no sea GCC.

EDIT: Olvidé mencionarlo. __func__ almacena el nombre de la función actual. Ahora que lo pienso, quizá no sirva en C++, dado que los nombres de los métodos y funciones son modificados para hacer la sobrecarga de funciones...

ficion commented 10 years ago

Efectivamente, en GCC se pueden definir macros en la instrucción de compilación (con el parámetro -D MACRO=DEFINICION), y, por extensión, funciona en los Makefile y por lo tanto en CMake. Pero, no sé si VS tendría un forma de hacer lo mismo (debería poderse, vaya).

EDIT: En CMake se podemos usar usar: add_definitions( -D_FILENAME=... ) Para que defina el macro _FILENAME.

No sé con qué definirlo, pero eso creo que sería. :P

adrigm commented 10 years ago

@InExtremaRes, puedes hacer pull request. Habría que cambiar todas las cadenas de log existentes al nuevo formato.

ArnauPrat commented 10 years ago

Hola, solo un par de preguntas que me surgieron mirando el código.

Gracias

InExtremaRes commented 10 years ago

Lamento la tardanza en mi respuesta, pero hay días como hoy que estoy todo el día afuera (estudios, trabajo, etc).

@adrigm Entre hoy y mañana ajustaré algunos detalles y haré el pull request. Me tomé la libertad de modificar los logs existentes (hasta el momento de la implementación) para que usaran el nuevo formato y no fallara la compilación. Lo que sí, esto dejó un commit bastante grande. ¿Prefieres que los divida en 2 antes de hacer el pull request? Lo malo, el commit con la implementación por sí solo romperá la compilación, porque modifica la clase Log.

@ArnauPrat La primera duda que planteas es muy interesante. La verdad, copié la implementación anterior del método Log::init() y no puse ningún mecanismo de control de errores. Normalmente este tipo de cosas es propiedad de las excepciones, pero no sé que mecanismo se usará en el proyecto (sin contar que mucha gente le tiene gran temor a las excepciones [y a veces con razón]).

Respecto a lo segundo: El destructor de Log es quien se encarga de escribir en el archivo, pero esto se hace por cada llamada a GDE_LOG_INFO (o debug, warning, etc). Por tanto, la única razón para que el motor se caiga y no se escriba el log es que el fallo esté en la misma función de Log. Si se cae antes de llegar al destructor, no se escribirá esa línea en particular, pero sí todas las líneas anteriores.

El método de funcionamiento es básicamente: crear un objeto temporal de tipo Log (sin nombre), la clase utiliza operator<< y std::streamstring para construir una cadena, cuando aparece el ; el objeto temporal ya no es necesario y se destruye escribiendo la línea en el archivo.

Saludos.

adrigm commented 10 years ago

Puedes hacerlo en un commit aunque sea grande hacerlo en dos va a romper la compilación y no va a aportar claridad. Como cambiamos el sistema de log se ve en el commit que cambia la interfaz pública y queda más claro para futuras revisiones de ese commit.

InExtremaRes commented 10 years ago

Perfecto. Una última duda. Creé macros de la forma GDE_LOG_INFO, GDE_LOG_DEBUG, etc para los distintos niveles, más que nada para una futura opción de eliminar los mensajes en ciertos builds. Lamentablemente, si se utiliza una opción como la que da @rickyah en el issue #20, de utilizar la macro de modo que no se expanda en nada, la interfaz pública debería cambiar otra vez, dejándo algo como:

GDE_LOG_INFO("etiqueta", "mensajes" << "de" << "salida");

en vez de la actual sintaxis:

GDE_LOG_INFO("etiqueta") << "mensajes" << "de" << "salida";

¿espero a que se tome una decisión respecto a esto o hago el pull request ya?

adrigm commented 10 years ago

Vamos a esperar a que opinen los demás. Aunque en la fase que estamos asumimos que la interfaz pública va a estar cambiando constantemente.

Yo voto por cambiar para evitar que se expanda la macro.

A modo de comentario pregunto si es necesario tener un parámetro etiqueta. Podríamos asumir que la primera entrada al stream es la etiqueta y en caso de no quererla usar algún tipo de función estilo lo que haces con los espacios. Esto es solo una opinión. Por evaluar posibilidades.

InExtremaRes commented 10 years ago

Sip, me gusta esa idea. Podría quedar algo así:

Texto con etiqueta:

GDE_LOG_INFO("etiqueta" << "mensaje" << "de" << "salida");

Texto sin etiqueta:

GDE_LOG_INFO(notag << "mensaje" << "de" << "salida");
rickyah commented 10 years ago

Yo opino al contrario: si queréis usar etiquetas con streams siempre usamos un stream manipulator, si no, no. Pero el uso de etiquetas me parece innecesario ahora mismo.

imasip commented 10 years ago

@InExtremaRes muy buen trabajo con el Log, y muy bien documentado! Con ficheros documentados a este niverl, da gusto aprender. Gracias!

GutierrezDev commented 10 years ago

WOW. Excelente @InExtremaRes. Ademas muy bien documentado para los novatos en C++ (yo) jejejejej.

InExtremaRes commented 10 years ago

@imasip @GutierrezDev Muchas gracias por sus comentarios :)

Lamento no haber continuado en los pasados días (la falta de tiempo) pero ahora mismo estoy corrigiendo algunas últimas cosas y haré el pull request para implementar los cambios al repositorio oficial.

InExtremaRes commented 10 years ago

Tengo una duda existencial ¿Qué diferencia hay entre los mensajes de tipo Info con los mensajes de tipo Debug? ¿Cuál de ellos es más importante? ¿Qué tipos de mensajes debería representar cada uno?

agabi10 commented 10 years ago

@InExtremaRes Si no me equivoco el de Debug sólo muestra los mensajes cuando se estás en modo Debug, mientras que el Info los muestra siempre, independientemente del modo.