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

[4.0] Rename 'PluginConfig' struct to fix lto warnings #45183

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

naithar
Copy link
Contributor

@naithar naithar commented Jan 14, 2021

Should cover C++ One Definition Rule warning for PluginConfig struct from: #45179

Added a prefixes for platform's PluginConfig struct and plugin constants to remove possible name collisions.

This changes are already cherry-picked for #41340

@@ -35,22 +35,22 @@
#include "core/io/config_file.h"
#include "core/string/ustring.h"

static const char *PLUGIN_CONFIG_EXT = ".gdap";
static const char *ANDROID_PLUGIN_CONFIG_EXT = ".gdap";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe those should be class constants so that they're properly namespaced? Then we don't need the suffix.

@@ -261,7 +261,7 @@ class EditorExportPlatformAndroid : public EditorExportPlatform {
EditorProgress *ep = nullptr;
};

Vector<PluginConfig> plugins;
Vector<AndroidPluginConfig> plugins;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd typically use PluginConfigAndroid to further specify PluginConfig (see EditorExportPlatformAndroid).

Which raises the question of whether this shouldn't be made abstract as a PlatformPluginConfig interface that each platform can implement, when relevant? Though that's outside the scope of this PR.

@naithar naithar force-pushed the fix/plugin-config-names branch from f91fde4 to eac1900 Compare January 14, 2021 23:17
@naithar
Copy link
Contributor Author

naithar commented Jan 14, 2021

@akien-mga fixed some of your comments, should be fine now.
Not sure about shared PlatformPluginConfig interface. Maybe we can implement it once more platforms support plugins, but for now I don't think it would give any benefit.

@@ -67,7 +50,24 @@ The `dependencies` section and fields are optional and defined as follow:

See https://github.com/godotengine/godot/issues/38157#issuecomment-618773871
*/
struct PluginConfig {
struct PluginConfigAndroid {
static const char *PLUGIN_CONFIG_EXT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to have the values declared inline with C++11 no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, got inline initializer error on compilation, so left it as it is now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://stackoverflow.com/a/1563906 this may work with inline in C++17 (which we use).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that it builds fine (with GCC at least):

diff --git a/platform/android/plugin/godot_plugin_config.h b/platform/android/plugin/godot_plugin_config.h
index 04091b49f7..a2ea42d311 100644
--- a/platform/android/plugin/godot_plugin_config.h
+++ b/platform/android/plugin/godot_plugin_config.h
@@ -51,7 +51,7 @@ The `dependencies` section and fields are optional and defined as follow:
  See https://github.com/godotengine/godot/issues/38157#issuecomment-618773871
  */
 struct PluginConfigAndroid {
-       static const char *PLUGIN_CONFIG_EXT;
+       inline static const char *PLUGIN_CONFIG_EXT = ".gdap";
 
        static const char *CONFIG_SECTION;
        static const char *CONFIG_NAME_KEY;
@@ -84,8 +84,6 @@ struct PluginConfigAndroid {
        Vector<String> custom_maven_repos;
 };
 
-const char *PluginConfigAndroid::PLUGIN_CONFIG_EXT = ".gdap";
-
 const char *PluginConfigAndroid::CONFIG_SECTION = "config";
 const char *PluginConfigAndroid::CONFIG_NAME_KEY = "name";
 const char *PluginConfigAndroid::CONFIG_BINARY_TYPE_KEY = "binary_type";

@naithar naithar force-pushed the fix/plugin-config-names branch from eac1900 to 7990c2c Compare January 15, 2021 14:54
@naithar
Copy link
Contributor Author

naithar commented Jan 15, 2021

Fixed. Not sure if 3.2 should get an inline change as it gives: warning: inline variables are a C++17 extension [-Wc++17-extensions]

@akien-mga akien-mga merged commit a96ee71 into godotengine:master Jan 15, 2021
@akien-mga
Copy link
Member

Thanks!

Not sure if 3.2 should get an inline change as it gives: warning: inline variables are a C++17 extension [-Wc++17-extensions]

Indeed the 3.2 codebase is compiled with C++14 (while the code is mostly C++03 with some C++11 features, but we don't make any extensive use of new features there). So you might have to keep the non-inline version there.

@naithar naithar deleted the fix/plugin-config-names branch February 9, 2021 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LTO build failure on master branch with GCC 9.3.0 (warnings in Android/iOS plugin API and ICU codegen)
3 participants