Re: Small patch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <sven@xxxxxxxx>
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



Attachment: gegl-processor.diff
Description: Binary data

_______________________________________________
Gegl-developer mailing list
Gegl-developer@xxxxxxxxxxxxxxxxxxxxxx
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer

[Index of Archives]     [Yosemite News]     [Yosemite Photos]     [gtk]     [GIMP Users]     [KDE]     [Gimp's Home]     [Gimp on Windows]     [Steve's Art]

  Powered by Linux