widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

1) Is strange, but makes sense. I do not fully understand why that is needed now and wasn't before, but meh.

2) Dangerous assumption. The speed of a dynamic_cast() depends hugely on many factors. String comparison is always slow. See http://stackoverflow.com/questions/9778145/how-fast-is-dynamic-cast for a more involved discussion. 

What I do not like about the new design is that we got rid of the enum {} we had and now use strings to identify our classes everywhere. This is slow, in many places where a switch() got replaced with an if() else if() at least by an order of magnitude or two. I think we have to reconsider again. Also the comments in the old enum reads like we did if (type >= BUILDING) in places which I do not like design wise (fragile!) but which broke if we ever did. 

The best design imho would be to bring back the old enum but as an enum class (http://www.cprogramming.com/c++11/c++11-nullptr-strongly-typed-enum-class.html) that does not allow >= comparison and replace type_name() in the c++ world through type() everywhere. We then need a function that takes a type and returns a string (for Lua and probably for logging output, not sure).


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1341081/+merge/227107
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1341081.