Skip to content

Fix namespace isolation in GDCLASS macro#920

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
touilleMan:fix-using-godot-required
Nov 11, 2022
Merged

Fix namespace isolation in GDCLASS macro#920
akien-mga merged 1 commit into
godotengine:masterfrom
touilleMan:fix-using-godot-required

Conversation

@touilleMan

@touilleMan touilleMan commented Nov 11, 2022

Copy link
Copy Markdown
Member

fix #918

Note the second commit in this PR modify the test to remove the use of using namespace godot;, from my point of view this has both pros and cons:

  • pros: help detect namespace isolation leak like we saw here ^^
  • pros: further show the difference between macros and regular functions (given macros are totally separated from namespace), this is important for D_METHOD which looks like a macro but is in fact a method (hence the GDCLASS(...) vs godot::D_METHOD(...))
  • cons: the example is more verbose and less idiotic which is not great given it is the primary example one will refer to when learning Godot-CPP...

So what do you think ?
I guess the best of both worlds would be to have two separates folders: one for example and another for test (the latter typically being also run in the CI, which is something not currently done). However I'm personally too lazy for this change 😄

@akien-mga
akien-mga requested review from Faless and bruvzg November 11, 2022 16:16
@akien-mga akien-mga added bug This has been identified as a bug enhancement This is an enhancement on the current functionality labels Nov 11, 2022
@akien-mga

Copy link
Copy Markdown
Member

I think it's better if the example uses using namespace godot;, as it makes the code very similar to code in C++ modules, and being able to go easily from C++ module to GDExtension and back is a design goal for godot-cpp.

But indeed, we should probably consider separating the CI testing environment from the end user demo project. The current project was meant more for testing but ended up being used as reference demo.

@touilleMan
touilleMan force-pushed the fix-using-godot-required branch from 85f85ed to 2745d34 Compare November 11, 2022 16:26
@touilleMan

Copy link
Copy Markdown
Member Author

@akien-mga I've removed the second commit then 👍

@akien-mga akien-mga removed the enhancement This is an enhancement on the current functionality label Nov 11, 2022
@akien-mga
akien-mga merged commit bb5579d into godotengine:master Nov 11, 2022
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This has been identified as a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GDCLASS doesn't use godot namespace on StringName

2 participants