Fix some sign comparison warnings

[asmaloney]

Part of #999

[akien-mga]

This code should ideally be kept in sync with core/object/method_bind.h in Godot.

It doesn't raise a warning in Godot so I'm curious about what the difference is.

When we do need to cast, we prefer C-style casts ((int)array.size()) to using static_cast, mostly for consistency and stylistic reasons.

[Faless]

It doesn't raise a warning in Godot so I'm curious about what the difference is.

Upstream godot uses Vector<Variant> (so size() is int), while godot-cpp seems to use std::vector<Variant> (so size() is size_t).

Maybe it can be changed since we do have Vector templates in godot-cpp.

[asmaloney]

we prefer C-style casts

Ah. I looked at the style guide and did a search on the code base before I used static_cast and it's all over the code (1700+ instances). (Edit: In fact there are a whole bunch in this file already.)

I know it's a PIA, but all these code style "preferences" should probably be in the style guide if they are going to be enforced. It's hard to get a handle on what's "allowed" and what isn't (even looking at the source to try to maintain consistency).

[Faless]

Maybe it can be changed since we do have Vector templates in godot-cpp.

Well, this seems non trivial, so probably better left to a separate PR in case.

[akien-mga]

we prefer C-style casts

Ah. I looked at the style guide and did a search on the code base before I used static_cast and it's all over the code (1700+ instances). (Edit: In fact there are a whole bunch in this file already.)

I know it's a PIA, but all these code style "preferences" should probably be in the style guide if they are going to be enforced. It's hard to get a handle on what's "allowed" and what isn't (even looking at the source to try to maintain consistency).

That's fair, but it's not necessarily always well defined and thus easy to document. As you saw in the codebase static_cast<T> is indeed used quite a lot when dealing with classes and pointers. But for basic types likes int, float, or bool we're used to just doing C-style cast - not necessarily with a good reason, it's mostly the original preference from reduz and most contributors got used to it too :)

[asmaloney]

got used to it too

After 30ish years of C++ that's not going to happen for me - it will always feel unnatural. 😆

[Faless]

After 30ish years of C++ that's not going to happen for me - it will always feel unnatural.

Eheh, I think that is kinda the point of these conventions, you don't need 30 years of C++ experience to understand the godot codebase, which used to be slightly more than C with classes.

I like to believe that this is part of the reasons why godot has so many contributors unlike other big projects, i.e. that the average C or C++ course is enough to get you started understanding the codebase (at least, it worked for me when I started contributing in 2016 only knowing plain academic C).

I have to admit though, that not relying on C++ specific features seems to consistently make C++ veterans unhappy.

[asmaloney]

I have to admit though, that not relying on C++ specific features seems to consistently make C++ veterans unhappy.

Oh no! Don't get me wrong. I'm not a "use all the features" guy. I'm absolutely a "C with classes" guy. Templates and multiple inheritance are the devil's work. I think C++ has gotten way out of hand in its complexity (I enjoy go more now).

In this specific case though I would argue that telling new devs to "always use static_cast" is easier/clearer than "use it sometimes, but not others".

I'm all about simplicity, consistency, and using the basics of the language properly. e.g. the previous "= default" discussion. It's not complicated to understand and makes the intent immediately obvious. Another one is using virtual and override at the same time - why? etc..

Generally I think static analysis tools provide a decent set of guidelines (e.g. clang-tidy's "modernize", "readability", etc.) to cover the basics.

Edit:

I think that is kinda the point of these conventions

But they aren't written anywhere, so how do people learn them? The static_cast one is a good example. Maybe someone new to C++ will learn it naturally from the code base, but anyone with any experience at all needs some guidance on the expectations.

Last Thought 😄 - this is why something like clang-tidy is awesome. It standardize all this - even if you can't get quite what you want, it's close enough. Kind of like clang-format.

[Faless]

Another one is using virtual and override at the same time - why? etc..

That for example, comes from the times when some C++ compiler didn't properly support the override identifier (and we used virtual in place of it to signal to the reader it was overridden 😅 ).

That said your experience with modernization and consistency tools with "simplified C++" is welcome and it could be really beneficial to the project.

We have an old discussion on 4.0 code base modernization ( godotengine/godot#33027 ), I think we could do something similar for 4.1 (as a godot-proposal though), and as long as it stays simple and we can enforce changes with tools, most contributors will be happy to adapt.

[Faless]

Thanks!