Identifiers with underscore

[Trigve]

I just want to point out that identifiers beginning with underscore and big letter or have double underscores anywhere are reserved in C++ (see http://en.cppreference.com/w/cpp/language/identifiers).

There are multiple identifiers in godot-cpp such as _ArgCast<>, _godot_class_instance_func<>(), ___get_method_class_name<>(), ... .

[karroffel]

Huh, didn't know that. I might change that. But maybe not since it's not user facing and if something breaks then I can still patch it without breaking stuff. But thanks for letting me know!

[Trigve]

Usually, if one need to use/create some "implementation detail" in header files (as with using templates), detail namespace is used (e.g. namespace godot{ namespace detail { ... }}) for such a thing, no need to use underscore suffixes/prefixes.

[BastiaanOlij]

Is this still an issue?

[Zylann]

Double and triple underscores are still present, maybe we could use a godot::detail namespace instead? That would also clean up readability I think.

For single-underscore though, it's even used as convention in Godot engine so those shouldn't be a problem hopefully?

[HuntJSparra]

There are still several instances of this in GDNative (3.* branches) and GDExtension (4.* branches). GDExtension's instances are mostly macros while GDNative's are mostly structs and templates.

Godot's stance on this seems to be "we fix when it breaks", and that was the case with godotengine/godot#51452.

[dsnopek]

I think we did remove the double underscores for 4.x at some point. But, yeah, using a single leading underscore seems baked into Godot APIs.

[HuntJSparra]

With the exception of __swap_tmpl in defs.hpp and math.hpp, all the reserved identifiers in GDExtension today are inherited from Godot. In the last GDExtension meeting, if I remember correctly:

1. We agreed to not change this in GDNative because the risk of breaking existing GDNative extensions greatly outweigh the potential benefits.
2. We are not changing any instances inherited from Godot.

So, it sounds like this can be closed unless we want to address __swap_tmpl or there is a report of this causing issues in the wild.

[HuntJSparra]

__swap_tmpl was addressed by #1758.

[dsnopek]

Thanks for looking into it!