Use namespace in defs.hpp

[zhehangd]

Code in defs.hpp is not in namespace godot and I don't see any reason for that. Fixing the namesapce has no effect to the other files in the library as they are already in it. From users' perspective they will need to add prefix godot:: to real_t, IndexSequence, BuildIndexSequence if they don't use using namespace godot; (like me). This breaks the compatibility to the old versions but the impact should be small.

[dsnopek]

From users' perspective they will need to add prefix godot:: to real_t

I think this is enough reason to consider not doing this. real_t is not something I'd personally expect to have to add godot:: to.

The other two (IndexSequence and BuildIndexSequence) are probably only used internally, not by users, so I'd expect that to be fine.

[zhehangd]

I don't think real_t is any special compared to the other Godot types which are already in the namespace. Though backward compatibility is a thing but for users it is just a matter of adding "using godot::real_t" somewhere to painlessly fix it. I would consider namespace pollution as a bug and it is not too late to fix it.

[Ivorforce]

I'd rather have real_t in the Godot namespace too to be honest. When I first worked on GDExtensions i was not 100% clear on what the type was until i looked it up. Considering even types like ptrdiff_t are expected to be used from the std namespace, i don't think it's far fetched to require Godot:: namespace for real_t.

[Bromeon]

It's also possible to have an interim deprecated using declaration:

using real_t [[deprecated("Moved to godot namespace")]] = godot::real_t;

to ease migration, and remove it after some time.

[dsnopek]

It's also possible to have an interim deprecated using declaration:

using real_t [[deprecated("Moved to godot namespace")]] = godot::real_t;

Ok! I think I'd be willing to go along with this if we had compatibility per @Bromeon's suggestion above.

I wonder if we should have a DISABLED_DEPRECATED define, like in Godot itself? That's something for a future PR, though.

[zhehangd]

using real_t [[deprecated("Moved to godot namespace")]] = godot::real_t;

I just experimented with this idea and found one drawback. "using namespace godot" cannot eliminate this warning. Although I am not one of the fans of this style, but I think they have right to have a clean compilation log as they are not doing wrong.

namespace godot {
typedef float real_t;
}

using real_t [[deprecated("real_t has been moved to godot namespace")]]  = godot::real_t;

using namespace godot;

int main() {
    real_t a;
    return 0;
}

A compromise would be leaving this using statement without deprecation warning, so we have the namesapce and nothing breaks, and we may consider removing this orphan in the future.

[Ivorforce]

I did find another workaround: While deprecating real_t globally, we could add to the deprecation warning that users can use using godot::real_t; in addition to using namespace godot temporarily to suppress this warning. This would have to be upkept until we remove real_t for real.

Still, there is one drawback to moving real_t to the godot namespace, in that it's inconsistent with godot upstream, where it's in the global namespace. If any GDExtension wants to be compileable as a module, it will have to work around that.

[zhehangd]

I did find another workaround: While deprecating real_t globally, we could add to the deprecation warning that users can use using godot::real_t; in addition to using namespace godot temporarily to suppress this warning. This would have to be upkept until we remove real_t for real.

Still, there is one drawback to moving real_t to the godot namespace, in that it's inconsistent with godot upstream, where it's in the global namespace. If any GDExtension wants to be compileable as a module, it will have to work around that.

It indeed works, but I don't feel good about the idea of forcing people to write both "using namespace godot" and "using godot::real_t;", which is quite weird.

As for the inconsistency you mentioned, unlike godot-cpp, and not just real_t, the upstream simply has nothing in the namesapce at all, so it shouldn't be a concern.

[dsnopek]

A compromise would be leaving this using statement without deprecation warning, so we have the namesapce and nothing breaks, and we may consider removing this orphan in the future.

Yeah, I think keeping the using but removing the [[deprecated...]] (like in the current PR) makes sense.