Hi there,
in r13061 I added two template functions in order to replace the tools' casting macros.
You can see them in action in widgets/pencil-toolbar.cpp:110.
I also added some compile-time checking there which can be uncommented later.
As I just stumbled across this. In order to build clang from its svn source, you need a c++11-enabled compiler:
"This is expected to be the last release of LLVM which compiles using a C++98 toolchain. We expect to start using some C++11 features in LLVM and other sub-projects starting after this release. That said, we are committed to supporting a reasonable set of modern C++ toolchains as the host compiler on all of the platforms. This will at least include Visual Studio 2012 on Windows, and Clang 3.1 or GCC 4.7.x on Mac and Linux. The final set of compilers (and the C++11 features they support) is not set in stone, but we wanted users of LLVM to have a heads up that the next release will involve a substantial change in the host toolchain requirements."
Should I proceed replacing the casting-macros like this? Any suggestions?
Regards,
Markus
On 26-2-2014 20:41, Markus Engel wrote:
Hi there,
in r13061 I added two template functions in order to replace the tools' casting macros.
You can see them in action in widgets/pencil-toolbar.cpp:110.
I also added some compile-time checking there which can be uncommented later.
Looks good. I like the compile-time checks, why do you have them disabled?
Why the static_cast? Why is there a namespace "Tools" and another "Tool"? That does not look too good.
Perhaps make a templated cast function that works for all types? template<typename Derived, typename T> bool Inkscape::is_a(const T* t) { static_assert(std::is_base_of<T, Derived>(),... return dynamic_cast<const Derived*>(t) != NULL; }
Let's add a const-version of ::to aswell.
As I just stumbled across this... In order to build clang from its svn source, you need a c++11-enabled compiler:
"This is expected to be the last release of LLVM which compiles using a C++98 toolchain. We expect to start using some C++11 features in LLVM and other sub-projects starting after this release. That said, we are committed to supporting a reasonable set of modern C++ toolchains as the host compiler on all of the platforms. This will at least include Visual Studio 2012 on Windows, and Clang 3.1 or GCC 4.7.x on Mac and Linux. The final set of compilers (and the C++11 features they support) is not set in stone, but we wanted users of LLVM to have a heads up that the next release will involve a substantial change in the host toolchain requirements."
Should I proceed replacing the casting-macros like this? Any suggestions?
Perhaps I can motivate you to spend your energy now on fixing bugs, so we can get the release out the door? ;-) Then after release, we can bug everybody about getting C++11 build environments set up, flip the switch, and go wild refactoring. :P (this is absolutely my plan after release)
Cheers, Johan
Looks good. I like the compile-time checks, why do you have them disabled?
type_traits is c++11.
Why the static_cast?
GSLists store their data in void-pointers. dynamic_cast refuses to cast them.
Why is there a namespace "Tools" and another "Tool"? That does not look
too good. I'll change that later :) .
Perhaps make a templated cast function that works for all types?
I'd rather leave it in "Tool". For me it's important that code is easily readable.
Tool::is_a<Pen> is easier to read than
Inkscape::is_a<Tool, Pen>.
Besides that, you'd have the problem "Tool is_a Pen? Or Pen is_a Tool?".
These casts are inlined anyway :) .
Let's add a const-version of ::to aswell.
Yeah, I forgot that. This won't work without it.
Perhaps I can motivate you to spend your energy now on fixing bugs, so we
can get the release out the door? ;-) Well, currently I've got 4 in-progress bugs. I'm just not appearing in Bryce's list as I have to set them to "fix released" instead of "committed" as they are caused by my refactoring :( .
Regards,
Markus
Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Donnerstag, 27. Februar 2014 00:20 An: Markus Engel; 'Inkscape Devel List' Betreff: Re: Casting-macro replacement
On 26-2-2014 20:41, Markus Engel wrote:
Hi there,
in r13061 I added two template functions in order to replace the tools' casting macros.
You can see them in action in widgets/pencil-toolbar.cpp:110.
I also added some compile-time checking there which can be uncommented later.
Looks good. I like the compile-time checks, why do you have them disabled?
Why the static_cast? Why is there a namespace "Tools" and another "Tool"? That does not look too good.
Perhaps make a templated cast function that works for all types?
template<typename Derived, typename T>
bool Inkscape::is_a(const T* t) {
static_assert(std::is_base_of<T, Derived>(),... return dynamic_cast<const Derived*>(t) != NULL; }
Let's add a const-version of ::to aswell.
As I just stumbled across this. In order to build clang from its svn source, you need a c++11-enabled compiler:
"This is expected to be the last release of LLVM which compiles using a C++98 toolchain. We expect to start using some C++11 features in LLVM and other sub-projects starting after this release. That said, we are committed to supporting a reasonable set of modern C++ toolchains as the host compiler on all of the platforms. This will at least include Visual Studio 2012 on Windows, and Clang 3.1 or GCC 4.7.x on Mac and Linux. The final set of compilers (and the C++11 features they support) is not set in stone, but we wanted users of LLVM to have a heads up that the next release will involve a substantial change in the host toolchain requirements."
Should I proceed replacing the casting-macros like this? Any suggestions?
Perhaps I can motivate you to spend your energy now on fixing bugs, so we can get the release out the door? ;-) Then after release, we can bug everybody about getting C++11 build environments set up, flip the switch, and go wild refactoring. :P (this is absolutely my plan after release)
Cheers, Johan
On 27-2-2014 0:30, Markus Engel wrote:
Looks good. I like the compile-time checks, why do you have them disabled?
type_traits is c++11...
Don't I miss it already...
Why the static_cast?
GSLists store their data in void-pointers. dynamic_cast refuses to cast them.
I don't think we should build unsafe casts into the is_a function. Yes GSLists are type-unsafe and we should move away from using it. GSList smth = ... Tool::is_a<Pen>(static_cast<...>(smth)); Ugly, but better, I feel.
Why is there a namespace "Tools" and another "Tool"? That does not
look too good. I'll change that later J.
Perhaps make a templated cast function that works for all types?
I'd rather leave it in "Tool". For me it's important that code is easily readable.
Tool::is_a<Pen> is easier to read than
Inkscape::is_a<Tool, Pen>.
Besides that, you'd have the problem "Tool is_a Pen? Or Pen is_a Tool?".
These casts are inlined anyway J.
I don't understand your answer. With the function I proposed, template<typename Derived, typename T> bool is_a(const T* t) { //static_assert(std::is_base_of<T, Derived>(),... return dynamic_cast<const Derived*>(t) != NULL; }
is_a<Pen>(...) works fine. Or is_aTools::PenTool if needed.
You made a shorter version of dynamic_cast<...>(...) with some compile-time type checking, and it does not need to be duplicated (not worried about object code duplication, but about code text duplication).
Let's add a const-version of ::to aswell.
Yeah, I forgot that. This won't work without it.
Perhaps I can motivate you to spend your energy now on fixing bugs, so
we can get the release out the door? ;-) Well, currently I've got 4 in-progress bugs. I'm just not appearing in Bryce's list as I have to set them to "fix released" instead of "committed" as they are caused by my refactoring L.
Sorry, didn't mean to belittle your other work!
ciao, Johan
Regards,
Markus
*Von:*Johan Engelen [mailto:jbc.engelen@...2592...] *Gesendet:* Donnerstag, 27. Februar 2014 00:20 *An:* Markus Engel; 'Inkscape Devel List' *Betreff:* Re: Casting-macro replacement
On 26-2-2014 20:41, Markus Engel wrote:
Hi there, in r13061 I added two template functions in order to replace the tools' casting macros. You can see them in action in widgets/pencil-toolbar.cpp:110. I also added some compile-time checking there which can be uncommented later.Looks good. I like the compile-time checks, why do you have them disabled?
Why the static_cast? Why is there a namespace "Tools" and another "Tool"? That does not look too good.
Perhaps make a templated cast function that works for all types?
template<typename Derived, typename T>
bool Inkscape::is_a(const T* t) {
static_assert(std::is_base_of<T, Derived>(),... return dynamic_cast<const Derived*>(t) != NULL; }
Let's add a const-version of ::to aswell.
As I just stumbled across this... In order to build clang from its svn source, you need a c++11-enabled compiler: "This is expected to be the last release of LLVM which compiles using a C++98 toolchain. We expect to start using some C++11 features in LLVM and other sub-projects starting after this release. That said, we are committed to supporting a reasonable set of modern C++ toolchains as the host compiler on all of the platforms. This will at least include Visual Studio 2012 on Windows, and Clang 3.1 or GCC 4.7.x on Mac and Linux. The final set of compilers (and the C++11 features they support) is not set in stone, but we wanted users of LLVM to have a heads up that the next release will involve a substantial change in the host toolchain requirements." Should I proceed replacing the casting-macros like this? Any suggestions?Perhaps I can motivate you to spend your energy now on fixing bugs, so we can get the release out the door? ;-) Then after release, we can bug everybody about getting C++11 build environments set up, flip the switch, and go wild refactoring. :P (this is absolutely my plan after release)
Cheers, Johan
I think this custom cast is only necessary as long as there are any GLists present. After that we may just use dynamic_cast directly.
If the source pointers are correctly typed, the compiler will throw an error on any of the cases the two static_asserts handle.
Currently, they are only there to guard against mistakes in the static_cast to ToolBase*. That's why I directly added it there.
Hm, let me think. There will always be an error, it doesn't matter whether those static_asserts are there, does it?
The longer I look at these templates the more they look weird to me.
I don't understand your answer.
Nevermind. Today, neither do I :) . You're totally right, it looks better provided that these asserts really make sense. As you suggested, we should postpone this issue a bit.
Sorry, didn't mean to belittle your other work!
You didn't :) .
So... you're saying that our bugpoint counter should be higher?
Probably yes :D. I think it's zero at the moment. As soon as a bug report is set to "fix released", our bug team sets the targeted milestone to "none".
And I seem to be unable to get a list of these bugs out of Launchpad.
** Changed in: inkscape
Milestone: 0.91 => None
Regards,
Markus
Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Donnerstag, 27. Februar 2014 19:26 An: Markus Engel; 'Inkscape Devel List' Betreff: Re: AW: Casting-macro replacement
On 27-2-2014 0:30, Markus Engel wrote:
Looks good. I like the compile-time checks, why do you have them disabled?
type_traits is c++11.
Don't I miss it already...
Why the static_cast?
GSLists store their data in void-pointers. dynamic_cast refuses to cast them.
I don't think we should build unsafe casts into the is_a function. Yes GSLists are type-unsafe and we should move away from using it. GSList smth = ... Tool::is_a<Pen>(static_cast<...>(smth)); Ugly, but better, I feel.
Why is there a namespace "Tools" and another "Tool"? That does not look
too good. I'll change that later :) .
Perhaps make a templated cast function that works for all types?
I'd rather leave it in "Tool". For me it's important that code is easily readable.
Tool::is_a<Pen> is easier to read than
Inkscape::is_a<Tool, Pen>.
Besides that, you'd have the problem "Tool is_a Pen? Or Pen is_a Tool?".
These casts are inlined anyway :) .
I don't understand your answer. With the function I proposed, template<typename Derived, typename T> bool is_a(const T* t) { //static_assert(std::is_base_of<T, Derived>(),... return dynamic_cast<const Derived*>(t) != NULL; }
is_a<Pen>(...) works fine. Or is_aTools::PenTool if needed.
You made a shorter version of dynamic_cast<...>(...) with some compile-time type checking, and it does not need to be duplicated (not worried about object code duplication, but about code text duplication).
Let's add a const-version of ::to aswell.
Yeah, I forgot that. This won't work without it.
Perhaps I can motivate you to spend your energy now on fixing bugs, so we
can get the release out the door? ;-) Well, currently I've got 4 in-progress bugs. I'm just not appearing in Bryce's list as I have to set them to "fix released" instead of "committed" as they are caused by my refactoring :( .
Sorry, didn't mean to belittle your other work!
ciao, Johan
Regards,
Markus
Von: Johan Engelen [mailto:jbc.engelen@...2592...] Gesendet: Donnerstag, 27. Februar 2014 00:20 An: Markus Engel; 'Inkscape Devel List' Betreff: Re: Casting-macro replacement
On 26-2-2014 20:41, Markus Engel wrote:
Hi there,
in r13061 I added two template functions in order to replace the tools' casting macros.
You can see them in action in widgets/pencil-toolbar.cpp:110.
I also added some compile-time checking there which can be uncommented later.
Looks good. I like the compile-time checks, why do you have them disabled?
Why the static_cast? Why is there a namespace "Tools" and another "Tool"? That does not look too good.
Perhaps make a templated cast function that works for all types?
template<typename Derived, typename T>
bool Inkscape::is_a(const T* t) {
static_assert(std::is_base_of<T, Derived>(),... return dynamic_cast<const Derived*>(t) != NULL; }
Let's add a const-version of ::to aswell.
As I just stumbled across this. In order to build clang from its svn source, you need a c++11-enabled compiler:
"This is expected to be the last release of LLVM which compiles using a C++98 toolchain. We expect to start using some C++11 features in LLVM and other sub-projects starting after this release. That said, we are committed to supporting a reasonable set of modern C++ toolchains as the host compiler on all of the platforms. This will at least include Visual Studio 2012 on Windows, and Clang 3.1 or GCC 4.7.x on Mac and Linux. The final set of compilers (and the C++11 features they support) is not set in stone, but we wanted users of LLVM to have a heads up that the next release will involve a substantial change in the host toolchain requirements."
Should I proceed replacing the casting-macros like this? Any suggestions?
Perhaps I can motivate you to spend your energy now on fixing bugs, so we can get the release out the door? ;-) Then after release, we can bug everybody about getting C++11 build environments set up, flip the switch, and go wild refactoring. :P (this is absolutely my plan after release)
Cheers, Johan
On 2014-02-28 24:21 +0100, Markus Engel wrote:
On 2014-02-27 19:26 +0100, Johan Engelen wrote:
So... you're saying that our bugpoint counter should be higher?
Probably yes :D. I think it’s zero at the moment… As soon as a bug report is set to “fix released”, our bug team sets the targeted milestone to “none”.
And I seem to be unable to get a list of these bugs out of Launchpad.
** Changed in: inkscape
Milestone: 0.91 => None
@Bryce:
Bugs closed by Markus since start of bug hunt (2013-12-30):
2014-02-19 r13044: https://bugs.launchpad.net/inkscape/+bug/1254373 Bug #1254373 <switch> node does not update when deleting a child (rev >= 12532)
2014-02-23 r13051: https://bugs.launchpad.net/inkscape/+bug/1274659 Bug #1274659 trunk: selection cue can no longer be turned off per tool (rev >= 12532)
Bugs closed by others in the same time frame and marked as 'Fix released':
2014-02-07 r13006 https://bugs.launchpad.net/inkscape/+bug/1275443 Bug #1275443 Including picture results in crash
2014-02-24 r13054 https://bugs.launchpad.net/inkscape/+bug/1204193 Bug #1204193 It's not possible to change length of horizontal or vertical lines using middle handles
2014-02-25 r13059 https://bugs.launchpad.net/inkscape/+bug/1284039 Bug #1284039 After moving a text box away from their origin, they don't retain their position when saved on SVG files
Not included in the list above: regressions introduced after the bug hunt was started and fixed by the same dev within a couple of days after the original commit.
On Fri, Feb 28, 2014 at 01:14:38AM +0100, su_v wrote:
On 2014-02-28 24:21 +0100, Markus Engel wrote:
On 2014-02-27 19:26 +0100, Johan Engelen wrote:
So... you're saying that our bugpoint counter should be higher?
Probably yes :D. I think it’s zero at the moment… As soon as a bug report is set to “fix released”, our bug team sets the targeted milestone to “none”.
And I seem to be unable to get a list of these bugs out of Launchpad.
** Changed in: inkscape
Milestone: 0.91 => None
Yeah, and my script relies on the milestone being set to 0.91 to take note of the fixes. I'll manually include the following bugs.
Bryce
@Bryce:
Bugs closed by Markus since start of bug hunt (2013-12-30):
2014-02-19 r13044: https://bugs.launchpad.net/inkscape/+bug/1254373 Bug #1254373 <switch> node does not update when deleting a child (rev >= 12532)
2014-02-23 r13051: https://bugs.launchpad.net/inkscape/+bug/1274659 Bug #1274659 trunk: selection cue can no longer be turned off per tool (rev >= 12532)
Bugs closed by others in the same time frame and marked as 'Fix released':
2014-02-07 r13006 https://bugs.launchpad.net/inkscape/+bug/1275443 Bug #1275443 Including picture results in crash
2014-02-24 r13054 https://bugs.launchpad.net/inkscape/+bug/1204193 Bug #1204193 It's not possible to change length of horizontal or vertical lines using middle handles
2014-02-25 r13059 https://bugs.launchpad.net/inkscape/+bug/1284039 Bug #1284039 After moving a text box away from their origin, they don't retain their position when saved on SVG files
Not included in the list above: regressions introduced after the bug hunt was started and fixed by the same dev within a couple of days after the original commit.
On Feb 27, 2014, at 3:21 PM, Markus Engel wrote:
I think this custom cast is only necessary as long as there are any GLists present. After that we may just use dynamic_cast directly. If the source pointers are correctly typed, the compiler will throw an error on any of the cases the two static_asserts handle. Currently, they are only there to guard against mistakes in the static_cast to ToolBase*. That’s why I directly added it there.
Ideally we can get to the situation where we don't need dynamic_cast<> at all, and code will just work directly with standard C++ inheritance.
In the immediate term, one would like to perhaps just have
PenTool *pc = dynammic_cast<PenTool *>(desktop->event_context); if (pc) { ... } (note that the 'if' and check are dropped, and inspecting the returned pointer for validity then suffices).
Then we should look at cleaning things up so that random down-casting is no longer needed. Much of that is from a C approach to coding, and is not the most efficient for C++.
Ideally we can get to the situation where we don't need dynamic_cast<> at
all, and code will just work directly with standard C++ inheritance. Be careful, Johan's gonna laugh at you ;) . I said exactly that some time ago... look for "Code style question".
-----Ursprüngliche Nachricht----- Von: Jon Cruz [mailto:jon@...18...] Gesendet: Freitag, 28. Februar 2014 01:18 An: Markus Engel; Johan Engelen Cc: Inkscape Devel List Betreff: Re: [Inkscape-devel] Casting-macro replacement
On Feb 27, 2014, at 3:21 PM, Markus Engel wrote:
I think this custom cast is only necessary as long as there are any GLists
present. After that we may just use dynamic_cast directly.
If the source pointers are correctly typed, the compiler will throw an
error on any of the cases the two static_asserts handle.
Currently, they are only there to guard against mistakes in the
static_cast to ToolBase*. Thats why I directly added it there.
Ideally we can get to the situation where we don't need dynamic_cast<> at all, and code will just work directly with standard C++ inheritance.
In the immediate term, one would like to perhaps just have
PenTool *pc = dynammic_cast<PenTool *>(desktop->event_context); if (pc) { ... } (note that the 'if' and check are dropped, and inspecting the returned pointer for validity then suffices).
Then we should look at cleaning things up so that random down-casting is no longer needed. Much of that is from a C approach to coding, and is not the most efficient for C++.
On Feb 27, 2014, at 4:21 PM, Markus Engel wrote:
Ideally we can get to the situation where we don't need dynamic_cast<> at
all, and code will just work directly with standard C++ inheritance. Be careful, Johan's gonna laugh at you ;) . I said exactly that some time ago... look for "Code style question".
:-)
A bit part of the problem is that The C approach of GTK+ adds in the "is-a" checking on things. We often end up with things upside down from a C++ perspective. That could be part of the problem with getting a nice template. Since it is 'unnatural' C++, the code to pull it off is not as simple as we would like.
Instead of having low-level thing foo poke into bar which is a subclass of baz, we might want to have foo invoke a base 'state-has-changed' method of the parent class baz which in turn gets processed in bar via an overloaded virtual, etc.
On 20-9-2013 0:40, Markus Engel wrote:
Hi, as I wrote a few days ago: "If too many of these downcasts appear, it
looks
to me like a design error. There should probably be a virtual method
then."
Want to make a million virtual functions? ;)
:)
-----Ursprüngliche Nachricht----- Von: Jon Cruz [mailto:jon@...18...] Gesendet: Freitag, 28. Februar 2014 01:29 An: Markus Engel Cc: 'Johan Engelen'; 'Inkscape Devel List' Betreff: Re: [Inkscape-devel] Casting-macro replacement
On Feb 27, 2014, at 4:21 PM, Markus Engel wrote:
Ideally we can get to the situation where we don't need dynamic_cast<> at
all, and code will just work directly with standard C++ inheritance. Be careful, Johan's gonna laugh at you ;) . I said exactly that some time ago... look for "Code style question".
:-)
A bit part of the problem is that The C approach of GTK+ adds in the "is-a" checking on things. We often end up with things upside down from a C++ perspective. That could be part of the problem with getting a nice template. Since it is 'unnatural' C++, the code to pull it off is not as simple as we would like.
Instead of having low-level thing foo poke into bar which is a subclass of baz, we might want to have foo invoke a base 'state-has-changed' method of the parent class baz which in turn gets processed in bar via an overloaded virtual, etc.
On Feb 27, 2014, at 4:31 PM, Markus Engel wrote:
On 20-9-2013 0:40, Markus Engel wrote:
Hi, as I wrote a few days ago: "If too many of these downcasts appear, it
looks
to me like a design error. There should probably be a virtual method
then."
Want to make a million virtual functions? ;)
:)
:-)
I believe the key thought here would be "judicious"
If we pick the right ones, we only need a few. Pick the wrong ones and... Ouch!
_<
d'oh
On 27-2-2014 0:30, Markus Engel wrote:
Perhaps I can motivate you to spend your energy now on fixing bugs, so
we can get the release out the door? ;-) Well, currently I've got 4 in-progress bugs. I'm just not appearing in Bryce's list as I have to set them to "fix released" instead of "committed" as they are caused by my refactoring L.
So... you're saying that our bugpoint counter should be higher? It'd be great if we can move on from the regression bug phase (yes, the points are very important, not if the program works well ;) ;) :P :P )
Cheers, Johan
On Thu, Feb 27, 2014 at 09:53:08PM +0100, Johan Engelen wrote:
On 27-2-2014 0:30, Markus Engel wrote:
Perhaps I can motivate you to spend your energy now on fixing
bugs, so we can get the release out the door? ;-) Well, currently I've got 4 in-progress bugs. I'm just not appearing in Bryce's list as I have to set them to "fix released" instead of "committed" as they are caused by my refactoring L.
So... you're saying that our bugpoint counter should be higher? It'd be great if we can move on from the regression bug phase (yes, the points are very important, not if the program works well ;) ;) :P :P )
Thanks for pointing this out; I thought my query was including both fix committed and fix released, but I'll doublecheck that this weekend when I run the numbers again.
And +1 to anything that'll help us move on to the next phase, and get the release out the door! I've collected some ideas that might help us step things up a little next week (and am definitely open to more ideas!)
Bryce
participants (5)
-
Bryce Harrington -
Johan Engelen -
Jon Cruz -
Markus Engel -
su_v