Small patch
This discussion is connected to the gegl-developer-list.gnome.org mailing list which is provided by the GIMP developers and not related to gimpusers.com.
This is a read-only list on gimpusers.com so this discussion thread is read-only, too.
| Small patch | Henrik Akesson | 12 Mar 11:16 |
| Small patch | Sven Neumann | 12 Mar 11:46 |
| Small patch | Henrik Akesson | 12 Mar 12:43 |
| Small patch | Sven Neumann | 12 Mar 19:34 |
| Small patch | Martin Nordholts | 12 Mar 17:44 |
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
Small patch
Hi,
some comments on your patch:
+#include #include
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
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
Hi,
some comments on your patch:
+#include #include
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
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
Small patch
Hi,
On Thu, 2009-03-12 at 12:43 +0100, Henrik Akesson wrote:
Thank you for the comments, the patch has been updated accordingly.
I have committed your changes to trunk.
One question: sometimes I see some methods without the gegl_something_ prefix, does that signify something (like private method)?
Nothing but lazyness. Sometimes it may even be intentional. We don't all share the same ideas about coding style. But if such functions exist, then they should definitely be static.
Second question: what does BLIT mean?
I think it is used in GEGL in the sense of http://en.wikipedia.org/wiki/Bit_blit
Sven
