I was looking over things a little more, and I *think* I know a better approach to solve the warnings.
Once I started looking at things, I realized that in at least some of the cases we have an SPObject* we need to end up with. Conceptually that's a GTK+/Glib object, not a C++ one, so we just need to work in the right paradigm
My first thought was something like this for sp-flowtext.cpp:
void* rawptr = 0; SPObject* source_obj = 0: ... group->layout.getSourceOfCharacter( it, &rawptr, &span_text_start_iter); source_obj = reinterpret_cast<SPObject*>(rawptr);
However, Ralf pointed out that using a reinterpret_cast<> itself triggered that warning. After I had that kicking around in my head for a while, I finally realized what I was missing: SP_OBJECT()
We need to force an unknown pointer to be one to an SPObject. However, that's a Glib object, not a base C++ one. And we already have that macro to coerce an arbitrary pointer into a pointer to an SPObject. Plus it does all its magic in C-land, not C++ land. So using it should clear up the warnings, and also is the "proper" GTK+ thing to do.
However... since I'm only on gcc 3.3 here, can someone else try this to see if it cleans things up on gcc 4.1.x?
Thanks
(I'm expecting that at the worst the SP_OBJECT macro will have the warning pop back up. But if it does then we'll have a single point in the codebase to change to fix everything, instead of needing to maintain every place that other function is used)
Index: src/sp-flowtext.cpp =================================================================== --- src/sp-flowtext.cpp (revision 12065) +++ src/sp-flowtext.cpp (working copy) @@ -558,21 +558,23 @@ if (set_y) sp_repr_set_svg_double(span_tspan, "y", anchor_point [NR::Y]);
- union { SPObject *op; void *vp; } source_obj; + SPObject* source_obj = 0; + void* rawptr = 0; Glib::ustring::iterator span_text_start_iter; - group->layout.getSourceOfCharacter(it, &source_obj.vp, &span_text_start_iter); - gchar *style_text = sp_style_write_difference ((SP_IS_STRING(source_obj.vp) ? source_obj.op->parent : source_obj.op)->style, group->style); + group->layout.getSourceOfCharacter(it, &rawptr, &span_text_start_iter); + source_obj = SP_OBJECT(rawptr); + gchar *style_text = sp_style_write_difference ((SP_IS_STRING(source_obj) ? source_obj->parent : source_obj)->style, group->style); if (style_text && *style_text) { span_tspan->setAttribute("style", style_text); g_free(style_text); }
- if (SP_IS_STRING(source_obj.vp)) { - Glib::ustring *string = &SP_STRING(source_obj.vp)-
string;
+ if (SP_IS_STRING(source_obj)) { + Glib::ustring *string = &SP_STRING(source_obj)->string; union { SPObject *op; void *vp; } span_end_obj; Glib::ustring::iterator span_text_end_iter; group->layout.getSourceOfCharacter(it_span_end, &span_end_obj.vp, &span_text_end_iter); - if (span_end_obj.op != source_obj.op) { + if (span_end_obj.op != source_obj) { if (it_span_end == group->layout.end()) { span_text_end_iter = span_text_start_iter; for (int i = group-
layout.iteratorToCharIndex(it_span_end) - group- layout.iteratorToCharIndex(it) ; i ; --i)
On May 30, 2006, at 2:11 AM, Ralf Stephan wrote:
Jon Cruz, That works, I'm reverting now. ralf
Cool.
I dislike union tricks in general, but then again I dislike warnings left in the code. Given all things, I think in this case the SP_OBJECT () macro gives us the better code legibility.
However, if it comes down to a hotspot that is measured to give problems, then the union work-around would be more appropriate as long as it actually made a measured difference in performance testing. Just wanted to make sure that everyone else was aware of the factors I was considering, and that sometimes using the union like that might be warranted. (but again, we should strive for code legibility first)
I dislike union tricks in general, but then again I dislike warnings left in the code. Given all things, I think in this case the SP_OBJECT () macro gives us the better code legibility.
However, if it comes down to a hotspot that is measured to give problems, then the union work-around would be more appropriate as long as it actually made a measured difference in performance testing. Just wanted to make sure that everyone else was aware of the factors I was considering, and that sometimes using the union like that might be warranted. (but again, we should strive for code legibility first)
Good that you say that. I have looked at the code behind SP_OBJECT(), and they just move the problem into a function elsewhere. Also, this resolved only the cases casting from void* to SPObject*. For the rest, which is code in util/glib-list-iterators.h and gc-soft-ptr.h, the macro will not suffice.
ralf
participants (2)
-
Jon A. Cruz
-
Ralf Stephan