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