You sure it builds for you?

Now i get this:

Make error line 298: problem compiling: src/extension/implementation/script.cpp: In member function 'Glib::ustring Inkscape::Extension::Implementation::Script:: resolveInterpreterExecutable(const Glib::ustring&)': src/extension/implementation/script.cpp:161: error: ISO C++ forbids comparison b etween pointer and integer In file included from src/extension/implementation/script.cpp:43: src/util/glib-list-iterators.h: In member function 'T* const& Inkscape::Util::GS ListConstIterator<T*>::operator*() const [with T = SPItem]': src/extension/implementation/script.cpp:748: instantiated from here src/util/glib-list-iterators.h:47: warning: dereferencing type-punned pointer wi ll break strict-aliasing rules

-----Original Message----- From: Krzysztof Kosiński [mailto:tweenk.pl@...400...] Sent: zondag 28 februari 2010 23:45 To: Engelen, J.B.C. (Johan) Cc: inkscape-devel@lists.sourceforge.net Subject: Re: Build broken (bzr 9118)

Oops, didn't remove the cast - should work now Regards, Krzysztof

W dniu 28 lutego 2010 23:14 użytkownik <J.B.C.Engelen@...1578...> napisał:

...

After bzr 9118, I get the following build error on Windows XP:

============================= Make error line 298: problem compiling:

src/extension/implementation/script.cpp:

...

In member function 'Glib::ustring

Inkscape::Extension::Implementation::Script::

...

resolveInterpreterExecutable(const Glib::ustring&)': src/extension/implementation/script.cpp:160: error: invalid

conversion

...

from 'uns igned int' to 'HINSTANCE__*' src/extension/implementation/script.cpp:161: error: ISO C++ forbids comparison b etween pointer and integer In file included from src/extension/implementation/script.cpp:43: src/util/glib-list-iterators.h: In member function 'T* const& Inkscape::Util::GS ListConstIterator<T*>::operator*() const [with T = SPItem]': src/extension/implementation/script.cpp:748:   instantiated

from here

...

src/util/glib-list-iterators.h:47: warning: dereferencing

type-punned

...

pointer wi ll break strict-aliasing rules

Thanks for fixing it soon,  Johan

On Feb 28, 2010, at 4:18 PM, Krzysztof Kosiński wrote:

Now a cast to unsigned long was missing. I hope it works now. Sorry for problems.

W dniu 1 marca 2010 01:01 użytkownik Jon Cruz <jon@...18...> napisał:

...

I see you ignored the comment on that bug/patch about using C++ casts and not C casts. Are you familiar wit the difference between them?

Yes, I can change the snippet in script.cpp to reinterpret_cast if you like. I was just in a hurry to fix a build break and forgot about it. In any case, the relevant part of code calls C functions

Yes, but the key thing here is that we're in C++ code and using a C++ compiler.

Those things were added to C++ for safety, etc. We need to use them wherever possible.

One of the BIG things that gives us is the ability to search and spot bad casts, improper algorithms, etc.

Also... "in a hurry" is exactly when one *needs* to focus on quality and care. Otherwise you get in this situation where you have breakage on top of breakage that comes from being reactive.

Oh, and the root cause here is that NO casts to unsigned long should have been used. In Win64 you have the situation where Microsoft went with LLP64 instead of LP64 so that LONG is the same as INT (both stay 32-bit), while pointers such as HINSTANCE go to 64 bit.

I have a cleanup almost done. I'm checking a local compile and then I can check in.

On Feb 28, 2010, at 4:18 PM, Krzysztof Kosiński wrote:

Yes, I can change the snippet in script.cpp to reinterpret_cast if you like. I was just in a hurry to fix a build break and forgot about it.

On the cast itself... reinterpret_cast<> should be the cast of last resort. First always try static_cast<> to see if the code will compile with that. With primitives that will often work.

(of course the rule in general is to just remove casts and the need for them, but that's not what applies in this case)

On Feb 28, 2010, at 7:35 PM, Jon Cruz wrote:

On Feb 28, 2010, at 2:50 PM, <J.B.C.Engelen@...1578...> <J.B.C.Engelen@...1761....1578...> wrote:

...

In file included from src/extension/implementation/script.cpp:43: src/util/glib-list-iterators.h: In member function 'T* const& Inkscape::Util::GS ListConstIterator<T*>::operator*() const [with T = SPItem]': src/extension/implementation/script.cpp:748: instantiated from here src/util/glib-list-iterators.h:47: warning: dereferencing type-punned pointer wi ll break strict-aliasing rules

Interesting.

You've been tripped up by a nasty old-school C macro.

Also... it appears to be one of the legacy Sodipodi things. The macro *looks* like the common GTK ones, but drops all the type safety internal checks that the GTK ones perform.

So I think this might be a case where we just need to drop that macro's use overall.

Ok. I did a quick change purging that. Hopefully it will unblock the build a bit for you.

(general note: SP_OBJECT_ID() should have been dropped with the change to C++)

On Mar 1, 2010, at 5:09 AM, bulia byak wrote:

On Mon, Mar 1, 2010 at 12:21 AM, Jon Cruz <jon@...18...> wrote:

...

(general note: SP_OBJECT_ID() should have been dropped with the change to C++)

No it shouldn't - you can change the underlying definition if you want but I see no reason to remove the convenient macro.

Instead, we need more macros like this. For example if we had preference calls in macros, that would have saved a good deal of the pain of moving to the new prefs class.

No, we really need to get rid of all macros.

Instead we need to use C++ features. That is much of the point of the language.

Yes, we should encapsulate things. But no, we should not use macros to do so.

You do hit on a good point about the preferences. If we have things properly *encapsualted* then the move would be easier. Of course, I think some of the move there was done improperly which caused some of the pain. However, that is a separate discussion.

To take a look at this specific macro, it fetches the ID string of a given SP Object or SP Object subclass. The cleaner way to approach that is to add an accessor to SPObject itself:

public gchar const* getID() const;

Among other things, C++ inheritance and such will help us out. It will also make it easier for an IDE to assist the programmer. A macro will block an IDE from doing as much.

Using code goes from

SPItem* item; ... str += SP_OBJECT_ID(item);

to become

SPItem item; ... str += item->getID();

Just as that macro, the getID() method is declared in the sp-object.h file. And since it is an accessor method, it hides the actual member being used (it encapsulates it). Now we are free to change the underlying implementation without touching any of the using code. We can also change the implementation in just one specific subclass without affecting any others. It's extremely difficult to do that with a macro. Additionally, the compiler and IDE will be able to perform more checks, promote type safety, and make the code generally more legible. Legible code is maintainable code.