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