Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unreferenced named parameters warnings from GDCLASS macro usage #1541

Closed
Klaim opened this issue Aug 3, 2024 · 2 comments · Fixed by #1545
Closed

Unreferenced named parameters warnings from GDCLASS macro usage #1541

Klaim opened this issue Aug 3, 2024 · 2 comments · Fixed by #1545

Comments

@Klaim
Copy link
Contributor

Klaim commented Aug 3, 2024

Godot version

4.3-rc2 4.2.2

godot-cpp version

2bcc4bd23c4e6d7db1f7d5f68be9727ac920c70a

System information

Windows 11, AMD Ryzen 9 5900X, 96GB RAM

Issue description

In my project using godot-cpp, when I have a type using GDCLASS macro (from wrapped.hpp) I get (among other) the following warnings:

warning C4100: 'p_rval': unreferenced formal parameter
warning C4100: 'p_count': unreferenced formal parameter
warning C4100: 'data': unreferenced formal parameter
warning C4100: 'p_instance': unreferenced formal parameter
warning C4100: 'p_token': unreferenced formal parameter
warning C4100: 'p_binding': unreferenced formal parameter
warning C4100: 'p_instance': unreferenced formal parameter
warning C4100: 'p_token': unreferenced formal parameter
warning C4100: 'p_reference': unreferenced formal parameter
warning C4100: 'p_instance': unreferenced formal parameter
warning C4100: 'p_token': unreferenced formal parameter

I removed the source paths because it's just the source file where I defined a node-inheriting class and used GDCLASS as usual, it's irrelevant.

My warning flags are just /W4, which is recommended for new projects (and I dont intend to reduce that level, that warning is useful in my project's code).

Setting the include directory as "system" will not work because this is a macro injecting code in the user's sources.

This seems easy to fix if you either comment these names (which all seems part of the macro) OR by using [[maybe_unnamed]] on them.
However, when trying to make a PR , I realized that file might be generated through scripts because of the weird formatting, in which case the issue should be fixed in that generator instead, but I'm not totally sure. If you tell me to just comment the names directly in that header, I'll make a quick PR, otherwise I dont feel confident in fixing this quickly and correctly myself (yet).

Notes:

  • I can see the same issue with the same build configurations (so same flags) when using v4.2.2 instead of v4.3-rc2 so it's not a regression
  • This line in tests cmake scripts deactivates the warning about unnamed parameters, essentially hiding the issue which will still appear in user's projects: https://github.com/godotengine/godot-cpp/blob/master/test/CMakeLists.txt#L51
  • Still in tests cmake scripts, the following lines show that the issue has been intentionally side-stepped? I didnt follow the context so not sure what's happening here. https://github.com/godotengine/godot-cpp/blob/master/test/CMakeLists.txt#L55-L58
  • I understand that there is an ongoing effort to reduce the warnings coming from the library headers - however here it's coming from a macro which needs to be used in user's source code, which implies that it can never be hidden without the user having to deactivate the warning themselves. Because these warnings seems easy to fix specifically in that macro, I think it's worth fixing these as a separate issue from the other header-warnings issues.

Steps to reproduce

Just adding /W4 to GODOT_COMPILE_FLAGS at cmake configuration using Visual Studio 2022 and building the test/ project will reveal the issue (I did test that locally).

In general, in any user project building using msvc:

  • set warning level 4 (/W4) when compiling a source file
  • in that source file, use GDCLASS as expected, for example (from my project):
// ...

#include <godot_cpp/core/class_db.hpp>
#include <godot_cpp/classes/node.hpp>

// ...
class __symexport MEGAST_Driver
    : public godot::Node
{
    GDCLASS(MEGAST_Driver, godot::Node)  // HERE THE WARNINGS ARE GENERATED
public:
    // ...        
    
 };

(__symexport does the dllimport/export dance, ignore that)

Minimal reproduction project

N/A

@dsnopek
Copy link
Collaborator

dsnopek commented Aug 7, 2024

This is a silly warning, but I think it'd be fine to remove the names from the unused arguments in the GDCLASS() macro.

However, when trying to make a PR , I realized that file might be generated through scripts because of the weird formatting, in which case the issue should be fixed in that generator instead, but I'm not totally sure. If you tell me to just comment the names directly in that header, I'll make a quick PR

The wrapped.hpp file is not created by the code generator, so this would just be a matter of making a PR that modifies that file.

@Klaim
Copy link
Contributor Author

Klaim commented Aug 7, 2024

Ok I'm on it then 👍🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants