Page MenuHomePhabricator

warning in ToolManager
Closed, ResolvedPublic

Description

with gcc ... will be removed

Event Timeline

That's probably not a good fix as it will likely brake now on different calls to that function that still pass signed intergers. I would suggest to do a static_cast in the comparison instead.

shouldn't only unsigned ints be passed as idx?

In theory yes (and than the according size_type to be 100% correct), in reality it is very common to pass integers - unfortunately. I mean, you can go for the complete fix than and make sure that all the calls are correct as well and that may cascade further and further. Just saying, a cast would be the quikest okay-ish fix here. :)

shouldn't it now have a build error/warning if you call this with an signed int ... i mean if you say "do it" - i will ... i'm in an argumentative mood and don't really see the necessity ;)

Yes, it will. And that's why I said you probably have to fix more than just the single location. So if you're in an argumentative mood: unsigned int is only a little bit better than int, as it actually should be std::container<type>::size_type. That is the very correct type here. But you don't want to expose that to the public interface for two reasons: (1) you might get warnings at least on Windows when linking dynamically as it may break when different versions of the runtime are involved that aren't binary compatible. That's a quite common warning in MSVC. (2) It's super inconvenient as 99% of the time you are passing around signed/unsigned integers. And that's okay because we know most of the time that 32 bits is more than enough for our indexes. Even if they are signed. And this brings us back to convenience. All the parts in MITK involving tool IDs use signed integers for that. So that's what gets passed around. And now there's your new unsigned int method which makes it necessary to cast explicitly. That should be enough reasons to understand that the local cast is the preferred solution here.

I didn't notice that you already merged your branch. Dashboard is green again without any further changes. Lucky you! 🐝

now i do understand ... but yes, i had already merged before we started this conversation ;)
i'll do it right next time, thank you for explaining :)