Hi everyone,
have you worked your way through my changes yet? Any comments? :)
Krzysztof, you wanted to do the review?
Regards,
Markus
2013/9/14 Markus Engel <p637777@...1081...>:
Hi everyone,
have you worked your way through my changes yet? Any comments? J
Krzysztof, you wanted to do the review?
The changes seem mostly OK, but there are a lot of conflicts with the current trunk. It would help a lot if you updated the branch so that it merges cleanly.
I'm still not sure whether we want to wholesale abandon building a static library for inkscape / inkview. Probably the best solution would be to build a shared library, but with the toxic mess of Autotools it's not very easy.
Regards, Krzysztof
2013/9/14 Krzysztof Kosiński <tweenk.pl@...400...>:
2013/9/14 Markus Engel <p637777@...1081...>:
Hi everyone,
have you worked your way through my changes yet? Any comments? J
Krzysztof, you wanted to do the review?
The changes seem mostly OK, but there are a lot of conflicts with the current trunk. It would help a lot if you updated the branch so that it merges cleanly.
Oops, I didn't notice you already did that. :)
Here are some initial comments:
1. There are several coding style problems: - Whitespace does not conform to coding standards. We always use 4 spaces as the indent, while some of the files you've changed have inconsistent indent (sometimes 4 spaces, sometimes 1 tab). - We always write the reference and pointer signs attached to the variable name, not the type name. - We qualifiers such as const after the type name, not before it, because they bind to the right.
The first two problems can be fixed automatically by running astyle, but const placement needs to be fixed manually.
For reference: http://staging.inkscape.org/en/develop/coding-style/
2. SP_IS_TYPE() macros should be implemented as a comparison with NULL, so that you can't assign the result to a pointer. SP_TYPE() macros should be reimplemented as dynamic_cast so that they generate compile-time errors when something not derived from SPObject is passed.
Other than that I haven't found any significant problems yet; the code looks mergeable. I'll complete the review today or tomorrow.
Regards, Krzysztof
- There are several coding style problems:
- Whitespace does not conform to coding standards. We always use 4 spaces as the indent, while some of the files you've changed have inconsistent indent (sometimes 4 spaces, sometimes 1 tab).
- We always write the reference and pointer signs attached to the variable name, not the type name.
- We qualifiers such as const after the type name, not before it, because they bind to the right.
Okay, I'll look into that. Although I'm sure that most of the style issues existed before I touched the files ;) .
- SP_IS_TYPE() macros should be implemented as a comparison with NULL, so that you can't assign the result to a pointer. SP_TYPE() macros should be reimplemented as dynamic_cast so that they generate compile-time errors when something not derived from SPObject is passed.
Okay. We have to be careful, though, as dynamic_casts add some runtime overhead. After some more refactoring steps we hopefully can omit most of these type checks.
Other than that I haven't found any significant problems yet; the code looks mergeable. I'll complete the review today or tomorrow.
Sounds good.
As for the static library: If someone complains we can think of something else :) .
Regards, Markus
I have problems when compiling r12535 (but probably r12532 is the cause) under Windows XP:
cc : compile of build/obj/sp-skeleton.o required by source: src/sp-skeleton.cpp
...
Make error line 298: problem compiling: In file included from src/sp-skeleton.cpp:27:0: src/sp-skeleton.h:31:5: error: 'SPObjectClass' does not name a type src/sp-skeleton.cpp: In function 'GType sp_skeleton_get_type()': src/sp-skeleton.cpp:48:1: error: 'SP_TYPE_OBJECT' was not declared in this scope src/sp-skeleton.cpp: In function 'void sp_skeleton_class_init(SPSkeletonClass*)': src/sp-skeleton.cpp:53:5: error: 'SPObjectClass' was not declared in this scope src/sp-skeleton.cpp:53:20: error: 'sp_object_class' was not declared in this scope src/sp-skeleton.cpp:53:54: error: expected primary-expression before ')' token src/sp-skeleton.cpp:53:55: error: expected ';' before 'klass' src/sp-skeleton.cpp:55:1: error: expected primary-expression before '<<' token src/sp-skeleton.cpp:55:3: error: expected primary-expression before '<<' token src/sp-skeleton.cpp:55:5: error: expected primary-expression before '<<' token src/sp-skeleton.cpp:55:7: error: expected primary-expression before '<' token src/sp-skeleton.cpp:55:9: error: 'TREE' was not declared in this scope src/sp-skeleton.cpp:56:5: error: expected ';' before 'sp_object_class' src/sp-skeleton.cpp:57:1: error: expected primary-expression before '==' token src/sp-skeleton.cpp:57:3: error: expected primary-expression before '==' token src/sp-skeleton.cpp:57:5: error: expected primary-expression before '==' token src/sp-skeleton.cpp:57:7: error: expected primary-expression before '=' token src/sp-skeleton.cpp:58:5: error: 'skeleton_parent_class' was not declared in this scope src/sp-skeleton.cpp:58:44: error: expected primary-expression before ')' token src/sp-skeleton.cpp:58:45: error: expected ';' before 'g_type_class_peek_parent' src/sp-skeleton.cpp:61:1: error: expected primary-expression before '>>' token src/sp-skeleton.cpp:61:3: error: expected primary-expression before '>>' token src/sp-skeleton.cpp:61:5: error: expected primary-expression before '>>' token src/sp-skeleton.cpp:61:7: error: expected primary-expression before '>' token src/sp-skeleton.cpp:61:9: error: 'MERGE' was not declared in this scope src/sp-skeleton.cpp:61:15: error: 'SOURCE' was not declared in this scope src/sp-skeleton.cpp:62:5: error: expected ';' before 'sp_object_class' src/sp-skeleton.cpp: At global scope: src/sp-skeleton.cpp:51:1: warning: unused parameter 'klass' [-Wunused-parameter] src/sp-skeleton.cpp: In function 'void sp_skeleton_build(SPObject*, SPDocument*, Inkscape::XML::Node*)': src/sp-skeleton.cpp:83:1: error: expected primary-expression before '<<' token src/sp-skeleton.cpp:83:3: error: expected primary-expression before '<<' token src/sp-skeleton.cpp:83:5: error: expected primary-expression before '<<' token src/sp-skeleton.cpp:83:7: error: expected primary-expression before '<' token src/sp-skeleton.cpp:83:9: error: 'TREE' was not declared in this scope src/sp-skeleton.cpp:84:5: error: expected ';' before 'if' src/sp-skeleton.cpp:87:1: error: expected primary-expression before '==' token src/sp-skeleton.cpp:87:3: error: expected primary-expression before '==' token src/sp-skeleton.cpp:87:5: error: expected primary-expression before '==' token src/sp-skeleton.cpp:87:7: error: expected primary-expression before '=' token src/sp-skeleton.cpp:91:1: error: expected primary-expression before '>>' token src/sp-skeleton.cpp:91:3: error: expected primary-expression before '>>' token src/sp-skeleton.cpp:91:5: error: expected primary-expression before '>>' token src/sp-skeleton.cpp:91:7: error: expected primary-expression before '>' token src/sp-skeleton.cpp:91:9: error: 'MERGE' was not declared in this scope src/sp-skeleton.cpp:91:15: error: 'SOURCE' was not declared in this scope src/sp-skeleton.cpp:109:1: error: expected ';' before '}' token src/sp-skeleton.cpp: At global scope: src/sp-skeleton.cpp:80:1: warning: unused parameter 'document' [-Wunused-parameter] src/sp-skeleton.cpp:80:1: warning: unused parameter 'repr' [-Wunused-parameter] src/sp-skeleton.cpp: In function 'void sp_skeleton_release(SPObject*)': src/sp-skeleton.cpp:1 cc : compile of build/obj/display/cairo-utils.o required by source: src/display/cairo-utils.cpp
And:
21:11: error: 'SPObjectClass' was not declared in this scope src/sp-skeleton.cpp:121:26: error: expected primary-expression before ')' token src/sp-skeleton.cpp:121:28: error: expected ')' before 'sp_skeleton_parent_class' src/sp-skeleton.cpp:122:70: error: expected ')' before ';' token src/sp-skeleton.cpp:122:70: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] src/sp-skeleton.cpp: In function 'void sp_skeleton_set(SPObject*, unsigned int, const gchar*)': src/sp-skeleton.cpp:136:11: error: 'SPObjectClass' was not declared in this scope src/sp-skeleton.cpp:136:26: error: expected primary-expression before ')' token src/sp-skeleton.cpp:136:28: error: expected ')' before 'sp_skeleton_parent_class' src/sp-skeleton.cpp:139:1: error: expected ')' before '}' token src/sp-skeleton.cpp:139:1: error: expected primary-expression before '}' token src/sp-skeleton.cpp:139:1: error: expected ';' before '}' token src/sp-skeleton.cpp: In function 'void sp_skeleton_update(SPObject*, SPCtx*, guint)': src/sp-skeleton.cpp:157:11: error: 'SPObjectClass' was not declared in this scope src/sp-skeleton.cpp:157:26: error: expected primary-expression before ')' token src/sp-skeleton.cpp:157:28: error: expected ')' before 'sp_skeleton_parent_class' src/sp-skeleton.cpp:160:1: error: expected ')' before '}' token src/sp-skeleton.cpp:160:1: error: expected primary-expression before '}' token src/sp-skeleton.cpp:160:1: error: expected ';' before '}' token src/sp-skeleton.cpp: At global scope: src/sp-skeleton.cpp:145:1: warning: unused parameter 'ctx' [-Wunused-parameter] src/sp-skeleton.cpp: In function 'Inkscape::XML::Node* sp_skeleton_write(SPObject*, Inkscape::XML::Document*, Inkscape::XML::Node*, guint)': src/sp-skeleton.cpp:181:11: error: 'SPObjectClass' was not declared in this scope src/sp-skeleton.cpp:181:26: error: expected primary-expression before ')' token src/sp-skeleton.cpp:181:28: error: expected ')' before 'sp_skeleton_parent_class' src/sp-skeleton.cpp:185:5: error: expected ')' before 'return' src/sp-skeleton.cpp:185:16: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] src/sp-skeleton.cpp:186:1: error: no return statement in function returning non-void [-Werror=return-type] cc1plus.exe: some warnings being treated as errors
Luca
-- View this message in context: http://inkscape.13.x6.nabble.com/Cppify-Branch-Review-tp4967928p4967992.html Sent from the Inkscape - Dev mailing list archive at Nabble.com.
I have problems when compiling r12535 (but probably r12532 is the cause)
under Windows XP:
Hi Luca, you may safely delete any content in these two files. sp-skeleton.h and sp-skeleton.cpp were files one could copy to create their own class that derives from SPObject. But as the name says, they are just templates with no actual functionality. They are of no use anymore and actually I wonder why these files are compiled anyway.
Regards, Markus
2013/9/19 Markus Engel <p637777@...1081...>:
I have problems when compiling r12535 (but probably r12532 is the cause)
under Windows XP:
Hi Luca, you may safely delete any content in these two files. sp-skeleton.h and sp-skeleton.cpp were files one could copy to create their own class that derives from SPObject. But as the name says, they are just templates with no actual functionality. They are of no use anymore and actually I wonder why these files are compiled anyway.
True, these are just example files, which should never have been included in the compilation. Try adding: <exclude name="src/sp-skeleton.cpp"/> <exclude name="src/sp-skeleton.h"/> to build.xml after line 343.
Regards, Krzysztof
Ok, I see. Anyway, there's no point for me to make such changes in my checkout (I don't have any urge to compile now); the useless files should be removed from bazaar control and build.xml modified on trunk.
Luca
P.S.: the build I launched before lunch (after latest devlibs update to r47) has just finished and the errors have disappeared. Now I have only the same error reported by Nicolas Dufour.
-- View this message in context: http://inkscape.13.x6.nabble.com/Cppify-Branch-Review-tp4967928p4967998.html Sent from the Inkscape - Dev mailing list archive at Nabble.com.
On 19-9-2013 13:48, Krzysztof Kosiński wrote:
2013/9/19 Markus Engel <p637777@...1081...>:
I have problems when compiling r12535 (but probably r12532 is the cause)
under Windows XP:
Hi Luca, you may safely delete any content in these two files. sp-skeleton.h and sp-skeleton.cpp were files one could copy to create their own class that derives from SPObject. But as the name says, they are just templates with no actual functionality. They are of no use anymore and actually I wonder why these files are compiled anyway.
True, these are just example files, which should never have been included in the compilation. Try adding: <exclude name="src/sp-skeleton.cpp"/> <exclude name="src/sp-skeleton.h"/> to build.xml after line 343.
I agree with the removal of sp-skeleton.cpp but not with the idea that such files should not be included when compiling. A still useful skeleton file is /src/live_effects/lpe-skeleton.h/.cpp. The reason I'd like to keep them being compiled is simply that otherwise those example files will rot and become terrible examples. Right? The LPE skeleton file is compiled on Windows, shall I add it to CMake and Autobuilds too? Just trying to keep you guys honest :P)
Cheers, Johan
Hi,
De : LucaDC <dicappello@...2144...> I have problems when compiling r12535 (but probably r12532 is the cause) under Windows XP: cc : compile of build/obj/sp-skeleton.o required by source: src/sp-skeleton.cpp
Confirmed. And r12531 also fails with the following error (r12530 compiles correctly): ---
Make error line 298: problem compiling: src/display/cairo-utils.cpp: In member function 'const gucha r* Inkscape::Pixbuf::getMimeData(gsize&, std::string&) const': src/display/cairo-utils.cpp:402:102: error: invalid conversion from 'gsize* {aka unsigned int*}' to 'long unsigned int*' [-fpermissive] c:\devlibs/include/cairo/cairo.h:2169:1: error: initializing argument 4 of 'void cairo_surface_get _mime_data(cairo_surface_t*, const char*, const unsigned char**, long unsigned int*)' [-fpermissive]
-- Nicolas
2013/9/19 Nicolas Dufour <nicoduf@...48...>:
De : LucaDC <dicappello@...2144...> I have problems when compiling r12535 (but probably r12532 is the cause) under Windows XP: cc : compile of build/obj/sp-skeleton.o required by source: src/sp-skeleton.cpp
Confirmed. And r12531 also fails with the following error (r12530 compiles correctly):
Make error line 298: problem compiling: src/display/cairo-utils.cpp: In member function 'const gucha r* Inkscape::Pixbuf::getMimeData(gsize&, std::string&) const': src/display/cairo-utils.cpp:402:102: error: invalid conversion from 'gsize* {aka unsigned int*}' to 'long unsigned int*' [-fpermissive] c:\devlibs/include/cairo/cairo.h:2169:1: error: initializing argument 4 of 'void cairo_surface_get _mime_data(cairo_surface_t*, const char*, const unsigned char**, long unsigned int*)' [-fpermissive]
Both problems should be fixed now.
Regards, Krzysztof
It seems that the changes in r12532 cause test compilation failures. Markus, can you provide a patch which would fix running "make check"?
Regards, Krzysztof
It seems that the changes in r12532 cause test compilation failures. Markus, can you provide a patch which would fix running "make check"?
I'll have a look. By the way, are you sure that the change you made in r12543 is correct? https://developer.gnome.org/gdk-pixbuf/stable/gdk-pixbuf-Image-Data-in-Memor y.html#gdk-pixbuf-copy This page tells me that g_object_unref would have been perfectly fine. Anyway, the pixbuf is allocated using "malloc", so I think "delete" is not the right solution, is it?
Regards, Markus
2013/9/19 Markus Engel <p637777@...1081...>:
By the way, are you sure that the change you made in r12543 is correct? https://developer.gnome.org/gdk-pixbuf/stable/gdk-pixbuf-Image-Data-in-Memor y.html#gdk-pixbuf-copy This page tells me that g_object_unref would have been perfectly fine. Anyway, the pixbuf is allocated using "malloc", so I think "delete" is not the right solution, is it?
"pb" has the type Inkscape::Pixbuf *, not GdkPixbuf *.
Regards, Krzysztof
participants (5)
-
Johan Engelen -
Krzysztof Kosiński -
LucaDC -
Markus Engel -
Nicolas Dufour