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 J . 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 J .
> 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 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_a<Tools::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@...360...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