[Gegl-developer] Small patch
Hi, here's a small patch on gegl-processor.c I've kept it very small in order to get some feedback on what I'm doing. The patch contains: - comments - refactoring of the render_rectangle function: extraction of code into a separate function get_band_size. No functional changes. - debug statements (well a few) I would especially like to get some feedback on the extraction (refactoring) of methods. There are some functions with about 300 lines, wich I wouldn't mind breaking up into smaller functions to make it easier to understand them. Thanks, Henrik gegl-processor.diff Description: Binary data ___ Gegl-developer mailing list Gegl-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
Re: [Gegl-developer] Small patch
Hi, some comments on your patch: +#include glib.h #include glib-object.h That's redundant as glib-object.h already includes this file for you. +inline gint get_band_size (gint size); Splitting the code into smaller functions is cool. But please consider to name the functions according to the file it lives in. So this would become gegl_processor_get_band_size(). This makes it easier to interpret stack traces and to discuss the code as it is clear where the function is located. +inline gint +get_band_size ( gint size ) Please format this as get_band_size (gint size). Also you will want to declare this function as static. And as far as I can see it should also be labeled G_GNUC_CONST as it doesn't examine any values except its parameters, and has no effects except its return value. I wouldn't declare it inline though. The compiler can better decide if it makes sense to inline this function. I am not happy about the way you reformatted the code in the class_init method: - g_object_class_install_property (gobject_class, PROP_NODE, - g_param_spec_object (node, -GeglNode, -The GeglNode to process (will saturate the provider's cache if the provided node is a sink node), -GEGL_TYPE_NODE, - G_PARAM_WRITABLE | - G_PARAM_CONSTRUCT)); + g_object_class_install_property ( +gobject_class, +PROP_NODE, +g_param_spec_object ( node, +GeglNode, +The GeglNode to process (will saturate the provider's +cache if the provided node is a sink node), +GEGL_TYPE_NODE, +G_PARAM_WRITABLE | +G_PARAM_CONSTRUCT)); Can we please keep the formatting here? If you want to reduce the code to fewer columns, you could break the blurb into multiple lines. Other than that, the patch looks good to me. If you incorporate my suggestions I will take care of getting the next version into trunk. Nice to see some work being done here. Sven ___ Gegl-developer mailing list Gegl-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
Re: [Gegl-developer] Small patch
Thank you for the comments, the patch has been updated accordingly. One question: sometimes I see some methods without the gegl_something_ prefix, does that signify something (like private method)? Second question: what does BLIT mean? Henrik 2009/3/12 Sven Neumann s...@gimp.org Hi, some comments on your patch: +#include glib.h #include glib-object.h That's redundant as glib-object.h already includes this file for you. +inline gint get_band_size (gint size); Splitting the code into smaller functions is cool. But please consider to name the functions according to the file it lives in. So this would become gegl_processor_get_band_size(). This makes it easier to interpret stack traces and to discuss the code as it is clear where the function is located. +inline gint +get_band_size ( gint size ) Please format this as get_band_size (gint size). Also you will want to declare this function as static. And as far as I can see it should also be labeled G_GNUC_CONST as it doesn't examine any values except its parameters, and has no effects except its return value. I wouldn't declare it inline though. The compiler can better decide if it makes sense to inline this function. I am not happy about the way you reformatted the code in the class_init method: - g_object_class_install_property (gobject_class, PROP_NODE, - g_param_spec_object (node, -GeglNode, -The GeglNode to process (will saturate the provider's cache if the provided node is a sink node), -GEGL_TYPE_NODE, - G_PARAM_WRITABLE | - G_PARAM_CONSTRUCT)); + g_object_class_install_property ( +gobject_class, +PROP_NODE, +g_param_spec_object ( node, +GeglNode, +The GeglNode to process (will saturate the provider's +cache if the provided node is a sink node), +GEGL_TYPE_NODE, +G_PARAM_WRITABLE | +G_PARAM_CONSTRUCT)); Can we please keep the formatting here? If you want to reduce the code to fewer columns, you could break the blurb into multiple lines. Other than that, the patch looks good to me. If you incorporate my suggestions I will take care of getting the next version into trunk. Nice to see some work being done here. Sven gegl-processor.diff Description: Binary data ___ Gegl-developer mailing list Gegl-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
Re: [Gegl-developer] Small patch
Sven Neumann wrote: Splitting the code into smaller functions is cool. But please consider to name the functions according to the file it lives in. So this would become gegl_processor_get_band_size(). This makes it easier to interpret stack traces and to discuss the code as it is clear where the function is located. Let's not forget the IMO most important aspect: being able to set breakpoints based on function name. If one does not namespace static functions it is likely that a project ends up with many different variants of the same static function. If it's a virtual method it's as good as inevitable. - Martin ___ Gegl-developer mailing list Gegl-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
