Use C++17 style static variable syntax #82127

jpakkane · 2023-09-22T16:20:14Z

This is the recommended way to do this since C++17. It also fixes a puzzling linker error if building with something other than SCons.

AThousandShips · 2023-09-22T16:24:15Z

if building with something other than SCons.

Which is it really something to consider? The code is built for SCons

ryanabx

The conversation of whether to make this change is still open, but my 2 cents is that this change is fine. I was the one who made that variable, and I apparently need to brush up on my modern C++ syntax 😅. If accepted, please cherry-pick the changes to 3.x, as the same variable exists there as well.

EDIT: Knowing now that there are other cases like this, for consistency, the PR should change the other cases as well. Then, a discussion will need to be had on the necessity of such a change.

AThousandShips · 2023-09-22T16:55:13Z

There's many cases like this, for example in ObjectDB

ryanabx · 2023-09-22T16:56:55Z

There's many cases like this, for example in ObjectDB

I wonder if his linker error crops up for the other cases as well. :/

AThousandShips · 2023-09-22T16:58:00Z

If this should be done I think it should be done properly

ryanabx · 2023-09-22T16:59:15Z

If this should be done I think it should be done properly

I think I agree. I was hastily under the impression that our code style involved using the newer syntax.

If there are equivalent cases to this one, they all should be changed to be inline in this PR.

AThousandShips · 2023-09-22T17:00:14Z

can someone find any cases of non-constant static variables defined this way
can someone confirm a need for this

ryanabx

I'd like to see all cases similar to this one changed to be inline, for consistency.

We also should continue discussing the necessity of such a change.

I think changing this would be fine if we can show there are no adverse effects. But we should remain consistent and use that style everywhere if we do make that change.

akien-mga · 2023-09-22T19:27:40Z

I think the linker error would be a sign of the buildsystem not having properly rebuilt all the objects that need to be rebuilt. Rebuilding from scratch might solve it. I don't see why the current syntax wouldn't work, and it's indeed used in several other places.

This specific one was added in #81844 merged recently.

jpakkane · 2023-09-22T22:45:10Z

I fixed the ones I could find with grepping. There were quite lot more of them than one would have expected.

This change is useful just on its own, because shows the default value in the header file rather than you having to go look it up in the corresponding cpp file.

I think the linker error would be a sign of the buildsystem not having properly rebuilt all the objects that need to be rebuilt.

No, that is not it. It is a complicated way in which compiler settings work with PIE in a way that I don't properly understand myself.

AThousandShips · 2023-09-23T07:52:37Z

This change is useful just on its own, because shows the default value in the header file rather than you having to go look it up in the corresponding cpp file.

On one hand yes, on another no, as it now makes changing the value affect the inheritance chain

AThousandShips · 2023-09-23T07:57:47Z

You're using spaces for indentation everywhere, you need to use tabs

AThousandShips · 2023-09-23T09:32:00Z

core/debugger/script_debugger.h

-	static thread_local int depth;
-	static thread_local ScriptLanguage *break_lang;
-	static thread_local Vector<StackInfo> error_stack_info;
+	static inline thread_local int lines_left = 0;


Suggested change

static inline thread_local int lines_left = 0;

static inline thread_local int lines_left = -1;

Make sure you copy the values correctly, there are other cases

AThousandShips · 2023-09-23T09:46:21Z

Still not complete, several of the touched files still have entries left in the previous style

AThousandShips · 2023-09-23T09:47:43Z

core/core_bind.h

@@ -249,7 +249,7 @@ class OS : public Object {
 class Geometry2D : public Object {
 	GDCLASS(Geometry2D, Object);

-	static Geometry2D *singleton;
+	static inline Geometry2D *singleton;


Several missed here

AThousandShips · 2023-09-23T09:48:27Z

core/io/ip.h

 	static void _bind_methods();

 	PackedStringArray _get_local_addresses() const;
 	TypedArray<Dictionary> _get_local_interfaces() const;

-	static IP *(*_create)();
+	static inline IP *(*_create)();


AThousandShips · 2023-09-23T09:48:48Z

core/io/packet_peer_dtls.h

@@ -38,10 +38,10 @@ class PacketPeerDTLS : public PacketPeer {
 	GDCLASS(PacketPeerDTLS, PacketPeer);

 protected:
-	static PacketPeerDTLS *(*_create)();
+	static inline PacketPeerDTLS *(*_create)();


AThousandShips · 2023-09-23T09:50:17Z

drivers/vulkan/vulkan_context.h

 	HashSet<CharString> enabled_instance_extension_names;

-	static bool device_extensions_initialized;
-	static HashMap<CharString, bool> requested_device_extensions;
+	static inline bool device_extensions_initialized;


AThousandShips · 2023-09-23T09:51:10Z

editor/editor_vcs_interface.h

@@ -95,7 +95,7 @@ class EditorVCSInterface : public Object {
 	};

 protected:
-	static EditorVCSInterface *singleton;
+	static inline EditorVCSInterface *singleton;


AThousandShips · 2023-09-23T09:51:28Z

editor/import/audio_stream_import_settings.h

@@ -81,7 +81,7 @@ class AudioStreamImportSettings : public ConfirmationDialog {

 	void _audio_changed();

-	static AudioStreamImportSettings *singleton;
+	static inline AudioStreamImportSettings *singleton;


AThousandShips · 2023-09-23T09:51:49Z

editor/node_dock.h

@@ -50,7 +50,7 @@ class NodeDock : public VBoxContainer {
 	Node *last_valid_node = nullptr;

 private:
-	static NodeDock *singleton;
+	static inline NodeDock *singleton;



AThousandShips · 2023-09-23T09:53:26Z

core/config/project_settings.cpp

@@ -49,8 +49,6 @@

 const String ProjectSettings::PROJECT_DATA_DIR_NAME_SUFFIX = "godot";



AThousandShips · 2023-09-23T09:54:00Z

core/input/input.cpp

@@ -72,8 +72,6 @@ static const char *_joy_axes[(size_t)JoyAxis::SDL_MAX] = {
 	"righttrigger",
 };

-Input *Input::singleton = nullptr;
-
 void (*Input::set_mouse_mode_func)(Input::MouseMode) = nullptr;
 Input::MouseMode (*Input::get_mouse_mode_func)() = nullptr;
 void (*Input::warp_mouse_func)(const Vector2 &p_position) = nullptr;


AThousandShips

Unable to go through all cases at the moment but needs a through check, also there's been some questions before I believe about the thread_local variables causing weirdness on some platforms so needs to be looked into

I don't see the direct value in changing the codebase when it works for the current build system, Godot currently only offers SCons and there's no solid plans to change it at the moment or offer alternatives, and this change risks introducing issues that can be hard to foresee

Edit: I also am not convinced that this is a code issue but that what ever other build system that has been set up doesn't match the options set up for SCons

jpakkane · 2023-09-23T10:14:43Z

This took so long that at some point I got fed up. I'll fix the rest only after there is a decision to actually merge this. I don't want to waste hours of my time on something that might be rejected outright.

AThousandShips · 2023-09-23T10:21:50Z

Understandable, but the change would need to be verified to actually work correctly so it's a bit complicated that way

Edit: I.e. it might end up not working out even after a decision to try it

jpakkane · 2023-09-23T11:04:59Z

There are comments in this thread that this should not be merged even if it works, because the old one is good enough. First a decision needs to be made that this will be merged if it works. Until that is decided, putting more effort into this is pointless.

akien-mga · 2023-09-27T08:31:00Z

I still don't understand how the original code would break compilation.

There's a lot of misunderstanding around buildsystems when treating them as black boxes, when at the end of the day all they do is call the compiler for each file with a list of options.

What option are you using in your Meson PR that differs from the SCons setup?

I'm not against modernizing usage (and indeed be careful around thread local stuff, that's touchy), but I need to know what we're fixing.

jpakkane · 2023-09-27T08:41:35Z

Without this change linking fails with this:

/usr/bin/ld: godot.p/main_main.cpp.o: warning: relocation against `_ZN22GDScriptLanguageServer13port_overrideE' in read-only section `.text'
/usr/bin/ld: godot.p/main_main.cpp.o: in function `Main::setup(char const*, int, char**, bool)':
/home/jpakkane/projects/build-godot-Desktop-debug/../godot/main/main.cpp:1534: undefined reference to `GDScriptLanguageServer::port_override'
/usr/bin/ld: warning: creating DT_TEXTREL in a PIE
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

Guessing this has something to do with how the toolchain wants to make some things position independent by default but not others.

akien-mga · 2023-09-27T08:55:22Z

The toolchain here should be GCC (or Clang) and GNU ld (or ldd), it shouldn't depend whether it's called by SCons or Meson/Ninja. So what I need to know is what you're configuring differently in Meson/Ninja that exposes this issue, so we can assess whether it's an actual bug in Godot or not.

So far to me this exposes a bug in the Meson/Ninja config which either removes or adds an option that Godot isn't meant to be compiled with.

If meson can generate a compilation database, it would be useful to compare with the one generated by SCons (with scons compiledb=yes).

stalkerg · 2023-09-29T10:51:40Z

I agree, you just should change Meson compiler options. It's not a Meson or source code issue, just Meson configuration.

jpakkane · 2023-09-29T12:37:32Z

The reason for the original failure turned out to be a bad #define setup. I have fixed that in my own branch.

jpakkane requested a review from a team as a code owner September 22, 2023 16:20

AThousandShips added enhancement discussion topic:codestyle labels Sep 22, 2023

AThousandShips added this to the 4.x milestone Sep 22, 2023

ryanabx approved these changes Sep 22, 2023

View reviewed changes

adamscott approved these changes Sep 22, 2023

View reviewed changes

adamscott added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 22, 2023

ryanabx suggested changes Sep 22, 2023

View reviewed changes

akien-mga removed cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 22, 2023

jpakkane force-pushed the newstyle_static branch from 7236b43 to 28b5b47 Compare September 22, 2023 22:45

jpakkane requested review from a team as code owners September 22, 2023 22:45

jpakkane force-pushed the newstyle_static branch from 28b5b47 to b898a6c Compare September 22, 2023 23:17

AThousandShips removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 23, 2023

jpakkane force-pushed the newstyle_static branch from b898a6c to d3eec71 Compare September 23, 2023 09:30

AThousandShips reviewed Sep 23, 2023

View reviewed changes

jpakkane force-pushed the newstyle_static branch from d3eec71 to db383e3 Compare September 23, 2023 09:37

Use modern style static variable declarations.

d01835a

jpakkane force-pushed the newstyle_static branch from db383e3 to d01835a Compare September 23, 2023 09:42

AThousandShips reviewed Sep 23, 2023

View reviewed changes

AThousandShips requested changes Sep 23, 2023

View reviewed changes

jpakkane mentioned this pull request Sep 27, 2023

Rebased Meson build system PR against current master #51153

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use C++17 style static variable syntax #82127

Use C++17 style static variable syntax #82127

jpakkane commented Sep 22, 2023

AThousandShips commented Sep 22, 2023

ryanabx left a comment •

edited

Loading

AThousandShips commented Sep 22, 2023

ryanabx commented Sep 22, 2023

AThousandShips commented Sep 22, 2023

ryanabx commented Sep 22, 2023 •

edited

Loading

AThousandShips commented Sep 22, 2023

ryanabx left a comment •

edited

Loading

akien-mga commented Sep 22, 2023

jpakkane commented Sep 22, 2023

AThousandShips commented Sep 23, 2023

AThousandShips commented Sep 23, 2023

AThousandShips Sep 23, 2023 •

edited

Loading

AThousandShips commented Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips Sep 23, 2023

AThousandShips left a comment •

edited

Loading

jpakkane commented Sep 23, 2023

AThousandShips commented Sep 23, 2023 •

edited

Loading

jpakkane commented Sep 23, 2023

akien-mga commented Sep 27, 2023

jpakkane commented Sep 27, 2023

akien-mga commented Sep 27, 2023 •

edited

Loading

stalkerg commented Sep 29, 2023

jpakkane commented Sep 29, 2023

	static inline thread_local int lines_left = 0;
	static inline thread_local int lines_left = -1;

		@@ -49,8 +49,6 @@

		const String ProjectSettings::PROJECT_DATA_DIR_NAME_SUFFIX = "godot";

Use C++17 style static variable syntax #82127

Are you sure you want to change the base?

Use C++17 style static variable syntax #82127

Conversation

jpakkane commented Sep 22, 2023

AThousandShips commented Sep 22, 2023

ryanabx left a comment • edited Loading

Choose a reason for hiding this comment

AThousandShips commented Sep 22, 2023

ryanabx commented Sep 22, 2023

AThousandShips commented Sep 22, 2023

ryanabx commented Sep 22, 2023 • edited Loading

AThousandShips commented Sep 22, 2023

ryanabx left a comment • edited Loading

Choose a reason for hiding this comment

akien-mga commented Sep 22, 2023

jpakkane commented Sep 22, 2023

AThousandShips commented Sep 23, 2023

AThousandShips commented Sep 23, 2023

AThousandShips Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

AThousandShips commented Sep 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AThousandShips left a comment • edited Loading

Choose a reason for hiding this comment

jpakkane commented Sep 23, 2023

AThousandShips commented Sep 23, 2023 • edited Loading

jpakkane commented Sep 23, 2023

akien-mga commented Sep 27, 2023

jpakkane commented Sep 27, 2023

akien-mga commented Sep 27, 2023 • edited Loading

stalkerg commented Sep 29, 2023

jpakkane commented Sep 29, 2023

ryanabx left a comment •

edited

Loading

ryanabx commented Sep 22, 2023 •

edited

Loading

ryanabx left a comment •

edited

Loading

AThousandShips Sep 23, 2023 •

edited

Loading

AThousandShips left a comment •

edited

Loading

AThousandShips commented Sep 23, 2023 •

edited

Loading

akien-mga commented Sep 27, 2023 •

edited

Loading