KeyWorksRW
Bug: Linking error due to one or more multiply defined symbols found for wxueImage() function
Description:
I have a solution where two distinct wxUiEditor projects are involved, even though I don't think two distinct projects matter.
Anyway, these two projects generate the function wxImage wxueImage(const unsigned char* data, size_t size_data) into two different cpp files.
This leads in a linking error ...
In my opinion the function wxueImage should be generated using the class name as its prefix; in my case:
wxImage wxhKeyDialogDesigner_wxueImage(const unsigned char* data, size_t size_data)
wxImage CKeyboardDesigner_wxueImage(const unsigned char* data, size_t size_data)
or as it was before, the function should be created as an inline member
inline wxImage wxueImage(const unsigned char* data, size_t size_data)
Unfortunately, using the inline keyword on a function requires a C++17 compiler. At one point I added a compiler_standard project property (PR #661) which then closed #657 which is essentially the same problem you are reporting. This resulted in non-compilable code for the standard gcc compiler that comes with Ubuntu 20.04 as well as the standard gcc compiler that comes with WSL2 on Windows 11. That resulted in issue #1285 closed by PR #1298 which removed the inline keyword.
The partial solution would be to re-enable the compiler version property, and if C++17 is enabled, the inline keyword would be used. The default would be C++11 since the inline isn't needed if there is only one project (for wxFormBuilder, this would mean importing all the projects into a single wxUiEditor project).
Unless C++17 was required, this solution still doesn't solve the problem for repositories like KiCad where even combining wxFormBuilder projects would still result in multiple wxUiEditor projects in multiple directories. Duplicate functions and duplicate data strings could still be created when the generated code from multiple wxUiEditor projects is compiled and linked into a single executable.
Ideally, wxUiEditor would know whether a single or multiple project's is going to used for a single executable. For a single project, that works today, with wxUiEditor itself being a good example (roughly 200 bitmaps, most which are used in more than one form). For multiple projects, the inline keyword would be a partial solution, and either class members or a static keyword (for both functions and bitmap data strings) used if C++17 isn't available.
Well, I understand ... but now I can't compile my projects because of an elementary issue. I can't avoid to share code among different executables, no one could achieve it.
The partial solution would be to re-enable the compiler version property, and if C++17 ...
Why don't you create wxueImage() function with a prefix using the class name where the function is used.
Or in my opinion, you should avoid to mix C style functions in object orientred code ... another chance is to create wxueImage() as private member of the class wxUiEditor is going to create.
These two options could solve forever this issue without involving neither C++ standard nor compiler.
Ideally, wxUiEditor would know whether a single or multiple project's is going to used for a single executable ...
Excuse me but it exist the shared code, often contained in custom libraries shared among different executables. In this case how could wxUiEditor recognize the presence of multiple projects in the same solution; following this aim the problem won't be solved.
Am I correct in assuming that for your projects, you are using a C++17 compliant compiler? If so, I'll add the inline keyword right away -- for older compilers, I want to think more about potential solutions that don't end up bloating the executable with duplicate images and functions.
Thank you @KeyWorksRW
Yes, I've been using inline since the C++11 standard.
Nowadays still C++11 is the standard used for my applications I build with GCC
Only for MSW executables, I build applications using C++14 standard.
GCC
Visual Studio
... please during your considerations, I pray you to keep in mind the opportunity, largely used by programmers, to create shared code among executables.
Because this is the main reason your actual vision, regarding the use of wxueImage() function, fails.
If I were you, I'd keep the suggestions I gave you above, or I'd try to create options to keep the user free to decide.
... I want to think more about potential solutions that don't end up bloating the executable with duplicate images and functions.
The suggestions I gave you won't' duplicate images, but only the functions to generate them. I can't imagine any other chance to permit shared code ...
Simply removing inline to avoid older compiler problems was a significant mistake on my part, and I'm sorry I ended up breaking your compiles -- and quite probably anybody else that doesn't use a single project. 😢
I want to provide a solution that solves your current scenarios while at the same time automatically supporting the updated functionality of the inline keyword in C++17 (potentially reducing executable size). I do not want to require you to have to use wxUiEditor to regenerate the files if the compiler changes.
The solution for the functions is to generate the following in every translation unit that calls the function:
#ifdef __cpp_inline_variables
inline wxImage wxueImage(const unsigned char* data, size_t size_data)
#else
static wxImage wxueImage(const unsigned char* data, size_t size_data)
#endif
{
// body of function
}
For C++14 and earlier, that means every module with this will have a unique copy of the function added to the executable. For C++17 and later, the linker will create a single function that all modules will call.
For the images themselves, the same applies, but now the defintion has to appear before it is used, so it will be at the top of the file instead of at the end of the file. The generated code will look like this:
namespace wxue_img
{
#ifdef __cpp_inline_variables
inline const unsigned char normal_png[508] {
#else
static const unsigned char normal_png[508] {
#endif
// data goes here
};
}
Like functions, if the same image is used in different classes, the data gets duplicated in the executable for C++14 and earlier. For C++17 and later, the linker will create a single copy of the data.
The unfortunate part of this is having that array of numbers at the top of the source file. It's not bad for dialogs, but for something like a ribbon panel like what wxUiEditor uses, that means dozens of image definitions to scroll through before you get to the actual function.
This will not be the case for the Image List node. The main purpose of that file is to place most if not all of your images in a single source file, which means that neither the functions or the image data can be defined as static, though it would be fine to define them as inline. That means you could only combine multiple projects with multiple image lists unless you either use a C++17 compliant compiler or you only place unique uses of an image in one of the Image List files.
Thank you @Randalphwa and please no worries.
I had only to modify manually the generated source code, in the meanwhile you can implement whatever you think would be useful; regarding the C++ standard, as I see it, it is not a problem for us to use the C++17 standard.
Perhaps even images definition couldn't be a big deal as nowadays IDE tools permit to fold code within a namespace.
What it is really important is to find a solution based on which you can implement your vision of things without breaking the "real world", which needs flexibility.
I would like to suggest a different style for the generated code you shared ...
#ifdef __cpp_inline_variables
#define WXUE_FNC_ATTRIBUTE inline
#define WXUE_IMG_ATTRIBUTE inline const
#else
#define WXUE_FNC_ATTRIBUTE static
#define WXUE_IMG_ATTRIBUTE static const
#endif
WXUE_FNC_ATTRIBUTE wxImage wxueImage(const unsigned char* data, size_t size_data)
{
// body of function
}
namespace wxue_img
{
WXUE_IMG_ATTRIBUTE unsigned char normal_png[508]
{
// data goes here
};
}
I build the wxWidgets library using the C++17 standard since wxWidgets 3.3 supports std::string_view if using C++17. For all of my own code including wxUiEditor, I require a C++20 compiler. Because I'm writing public facing code that anyone can read, I want to make the code as readable as possible, and I find that C++20 gives me the most options for writing concise code which might work the same as code written for older compilers, but is easier to read/understand. Or at least it is for someone familiar with C++20. I do have to be a bit careful -- I've already had one person who couldn't compile wxUiEditor because they are using a slightly older version of MSVC that had a bug that couldn't compile something I was using because that version of MSVC wasn't fully C++20 compliant. I'd also dearly love to use std::Format() -- but that requires gcc12 and a version of MSVC that is fully C++20 compliant.
Duplication of data or functions has been a problem for decades. For example, every time you #include a *.xpm file you are creating a unique copy of that data. Include it in more than one source file, and you get duplicates. The conflict is avoided because the data string is declared static. The change to the inline keyword was done to provide an exception to the "one definition rule" allowing you to include the same data and code into multiple source files while still only having a single version in the executable. My understanding is that the only way to avoid duplication prior to C++17 was to use global functions/data. The Image List in wxUiEditor essentially does that by moving all the functions and potentially all of the data into globals that any module can access by include the image file's header. However, that's useless if you are sharing generated code between different projects.
On Tuesday, December 5th, 2023 at 5:14 AM, KeyWorksRW @.***> wrote:
Am I correct in assuming that for your projects, you are using a C++17 compliant compiler? If so, I'll add the inline keyword right away -- for older compilers, I want to think more about potential solutions that don't end up bloating the executable with duplicate images and functions.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>
That's a really good idea for using the #defines to eliminate the conditionals -- thanks!
I build the wxWidgets library using the C++17 standard since wxWidgets 3.3 supports std::string_view if using C++17. For all of my own code including wxUiEditor, I require a C++20 compiler. Because I'm writing public facing code
Indeed a remarkable aim, I wish you all the best. Even though a "limit" in C++ standard, nowadays would be reasonable.
... may I ask you where are you from? (just for my curiosity)
There's a lot of code changes involved with this issue. I'm outlining the underlying assumptions here that the changes are revolving around:
- At code generation time wxUE does not know what compiler is being used -- compiler conditionals are used instead (via conditional macros). That means no compiler property (previously added, then removed).
- Each generated file needs to be self-contained unless there is an Images List file which contains one or more images that the generated file needs to reference.
- If C++17 is available, functions and image data will be marked as
inlineto prevent duplication if they are used elsewhere. - If the compiler is C++11 or C++14 then
wxueImage(),wxueBundleSVG()andwxueAnimation()will be marked as static and added to every generated file that needs them (including the Images List file). - The wxue_img::bundle...() functions in the Images List file will not be marked as
staticorinline. These functions are in a namespace and are considered global -- any class include the Images List header file will be able to use these functions irregardless of what compiler is being used.
If an image is added to the Images List file, then any class that uses that image will #include the Images List file and access the image via the wxue_img::bundle...() functions. Classes that need to be completely self contained should not place any images that they need to access in an Images List file. Creating an Images List assumes you want multiple classes within a project to use it (or you simply want all the images to be generated in a different file).
In terms of code changes, the following have either been made or are planned to be made:
- image location will no longer be tracked -- images will be duplicated rather than being referenced from a different class. The unfortunate side effect is that now all projects compiled with C++11 or C++14 will potentially have duplicate functions and image data in their executable. Previously, this did not happen, but the generated files were not interchangeable between C++ projects, and it assumed a single wxUE project per C++ project. Compiling with C++17 will prevent the duplication.
- Images defined in a source file will now appear at the top rather than the bottom of the file. That's required for the images to be declared as static. For any class with a toolbar or ribbon bar, this now means a lot of data to scroll through to get to where the class is actually created.
- Now that tracking where an image is going to be generated is no longer used to avoid duplication, updating the Image List to support
auto_addandcollect_allproperties should be fairly straightforard to finish implementing. When using a single wxUE project for a C++ project, this will effectively make the source files look the same as they are before these changes. - While you still can place an animation file into an Images List, the
wxueAnimation()function will only be used in the file with the animation control.
thank you @Randalphwa, it sounds good to me
For any class with a toolbar or ribbon bar, this now means a lot of data to scroll through to get to where the class is actually created.
Perhaps for a case like this the use of a file to include would help, but I know you don't like this way Anyway nowadays with modern IDE editors, code within a namespace can be folded
So the problem I ran into today is that this breaks one of my side projects, because I can no longer access either the bitmap or the function. C++17 doesn't help because you have to define the entire data array in the source file where you are using it. I can't do that in a non-generated file, because if the artwork ever changed, the copy of the generated data array would be wrong. For this project, I can make the data arrays in the Image List file non-static and they'll work fine. But for a user that didn't create an Image List file, derived classes can no longer access the data array in the generated class. So effectively, by making these data arrays static or inline, no user-created code can safely access them because you can no longer use an extern to reference them. It also means that if I drop in the planned changes, it will potentially break existing code that worked fine before the change (which I've now experienced myself).
Kind @Randalphwa ... checkmate!
This is a case where things must happen to understand the way must be taken :) Let me express my opinion, for what it worth, it's only an opinion and unfortunately no one else is participating at this discussion, but ...
I think the approach regarding resources in general, actually only images, is a bit wrong concerning the used programming style. Mainly for the following reasons, as I see it:
1st) which is the minor ... Why should I define an image in a class called CMyDialog1 and then use it in another class completely different called CMyFram2; I'm forcing the programmer to write a bad engineered code because I'm using CMyDialog1 as it was a resource dispenser globally visible, but it's only a kind of workaround to share resources.
2nd) which is indeed a bit dangerous
Generally speaking, mixing C style code in Object Oriented language is a bad habit, it can work of course, but it's a signal I'm doing something wrong.
Indeed I could solve my aim using a global object with static members instead of using functions defined everywhere which no one knows where they are defined; for what I learned using C++, when I feel like to use extern it's the signal I have to stop and start thinking something different.
The more the C++ standard will increase the more we should forget such habits I think.
Other case is when it is necessary to port an old code to a new one written with modern tools, but it is not your case.
The "list file" is something which is close to that scenario (above in bold), perhaps it is not the final solution ...
3rd) which is a bit incomprehensible to me you want to create a RAD tool which creates as few files as you can. OK a certainly approach based on which I could try to reduce such files could be good, but it could become a sort of cage. For example what's wrong with an object containing all project resources shared among classes for each wxUE projects.
You are doing a great job with wxUiEditor, stop for a while thinking how to handle resources using an Object Oriented style instead of insisting with functions defined randomly within the code. For sure it will potentially break existing code, but the benefit for the future of wxUE would be great, in my opinion, and users will be able to understand it and they will modify their code.
For sure you can't wait for years, but it should be something to be known by users, and your priority for the next weeks.
Regarding the existing code, you could leave things at they are, cimply using an option based on wich the user is free to generete code with inline functions or not, otherwise with the current development version no one will be able to share code among projects, as you know, and it is not acceptable at all.
The Image List file uses a namespace rather than a class, but essentially does the same thing -- with the exception of wxueImage() and wxueBundleSVG(), all functions and images within it are in the wxue_img namespace and it can be shared with any other class. In fact for wxWidgets 3.2, you don't even need those two functions -- you can just use the wxue_img::bundle...() functions for all SVG and bitmap images (PNG, JPEG, TIFF, etc.).
Using the Image List eliminates the problem with your 1) and 2) paragraph above -- if your dialog's images are placed into the Image List, then they become an external resource shared with any other form that needs them. For wxWidgets 3.2, you don't even need to know whether you are dealing with a single image or multiple images bundled together, and you don't need to know the size of the image data arrays. You simply call wxue_img::bundle...() and it returns a wxBitmapBundle and the caller doesn't need to know anything about the image data. If the image changes, only the Image List needs to be updated. There's only one module that contains the image data and the functions needed to access them. wxUiEditor forces that node to be at the top of your project to make it easy to preview the images as well as information about them.
There are extern statements in the Image List header file, but that's only for wxWidgets 3.1 users or for cases where a dev stores the image data in the Image List but isn't going to use it as a wxBitmapBundle. Typically this if for GIF files which might be used for wxAnimation, but they could also be used in a wxImage or wxBitmap. An ICO file is probably going to be used for wxIcon, but it could just as easily be used as a wxBitmap.
It's currently disabled, but there is code in place so that when you first create an Image List for a project, it asks if you want to move all of your images into the Image List. After that, any time you add an image to any form in your project, it is automatically added to the Image List file, and code generation will call a function in the wxue_img namespace to retrieve the wxBitmapBundle or wxImage. Neither the image data or the function definition will be in your form's class, nor will there by any image extern statements in your form's header file. There will be extern statements in the source file, but those are only there in case you need to access the image data directly (or you just want to know what images are being used by that form).
Short term I'm going to toss all the code I wrote this week (😱) and start over with a focus on fixing the problems with the function conflicts for projects not using an Image List, and changing the two functions in the Image List so that they are part of the wxue_img namespace. Until the alternate_name image property is finished, I can't really avoid image conflicts if you try to use the same image in two different wxUiEditor projects. If I make it static, it can break existing code -- same reason I can't make the data arrays in the Image List static. With alternate_name you could add the class name as a prefix to ensure the image name is unique (at the cost of duplicating the data array in the executable which is what happens now in all the other wxWidgets RAD tools).
Just as a note, the Image List is essentially a global resource as we've discussed a bit in #1269 -- though it doesn't yet do as much as you've suggested. It still needs a bunch of work which is why finding it is still a bit obscure (I'd be willing to bet that very few people even know it exists 😄 ).
The Image List file uses a namespace rather than a class, but essentially does the same thing
... you just don't like a single class :) It would be very useful to keep away from troubles and for implementing future features.
This is my idea, in short ...
Instead of wxBitmapBundle bundle_mybitmap_png(); a user should have something who permits to call
wxUeResources::GetBitmap("mybitmap");
wxUeResources::GetImage("mybitmap");
wxUeResources::GetBundle("mybitmap1", "mybitmap2", "mybitmap3", "mybitmap4", "mybitmap5", .... );
wxUeResources::GetBitmapFromSVG("mysvg", wxSize(48, 48)); ( in this case, with the opportunity to specify a size )
....
wxUeResources::Getxxxxxxxx("resource_name");
where xxxxxxxx is whatever you will be able to add as resource in the neasr future.
All resources, divided by kind of resource, images, SVG, sounds, etc ... are contained in different lists of same kind objects, reachable using their names.
It's currently disabled, but there is code in place so that when you first create an Image List for a project
I think it's time to restore that function :)
... I can't really avoid image conflicts if you try to use the same image ...
As I see it, it is up to the user avoid it
With alternate_name you could add the class name as a prefix to ensure the image name is unique (at the cost of duplicating the data array in the executable which is what happens now in all the other wxWidgets RAD tools).
It is not a big deal to me, indeed a nice opportunity for those who need it.
Just as a note, the Image List is essentially a global resource as we've discussed ...
Yes, but now it is detached by its potential consumers within the wxUE project, that's why probably it is not well known or used.
... you just don't like a single class :)
I expect a class to contain an object, not just a bunch of variables and functions. In this case the class would have no constructor or destructor since there is no object to create/destroy/modify/access. To me, creating a class that has no object is confusing for someone reading the code. Using a namespace makes it clear that it's just to avoid name conflicts. I.e., having a class wxUeResources would imply that you can create an instance of it via wxUeResources my_resources; which would definitely not be the case, again because there is no object to create.
wxUeResources::GetBundle("mybitmap1" "mybitmap2", "mybitmap3", "mybitmap4", "mybitmap5", .... );
That won't work. You have to specify the variable name, not the string name and you have to specify the size of the array. The function declaration would actually be:
wxBitmapBundle wxUeResources::GetBundle(const unsigned char* mybitmap1, size_t size_mybitmap1, const unsigned char* mybitmap2 = nullptr, size_t size_mybitmap2 = 0, const unsigned char* mybitmap3 = nullptr, size_t size_mybitmap3 = 0, const unsigned char* mybitmap4 = nullptr, size_t size_mybitmap4 = 0, const unsigned char* mybitmap5 = nullptr, size_t size_mybitmap5 = 0);
Versus the current:
wxBitmapBundle wxue_img::bundle_mybitmap1();
Granted the shorter form assumes you are following either of the two wxWidgets naming conventions for multi-dpi images.
Yes, but now it is detached by its potential consumers within the wxUE project
On the long-term list of things to look into is adding the ability to link an Image List as well as forms to a different wxUE project, sort of like the way sub modules work in github. You could then choose to generate your own copy of the code, or just assume you already have it to link to. You could potentially modify it (ala sub-modules style) after you confirmed multiple times that you are sure, really sure, and absolutely positively certain that you want to modify that external project. 😁
Adding the PR comments that changes this below. I'm leaving the issue open for now, since wxWidgets 3.1 code generation around images is still broken, and further testing is needed before I move the PR into the 1.2 branch for the next release. It should be in the 12/10 daily build. Use at your own risk...
This PR changes how the Image List file is generated, and how images are loaded with or without the existence of an Image File.
No Images List file
wxueImage() and wxueBundleSVG() will be generated in any file that needs them whether or not that file actually contains the image data array. A conditional is now used to mark the functions as inline if compiling under C++17 or later, or static if compiling under C++14 or earlier. For C++17, only one copy of the function will be linked into the executable, and for older compilers that static will prevent name conflicts, though it does mean the function body will be added multiple times to the executable.
Each image data array is still unique to the project, and where it gets defined still depends on the order of the forms in the project. Each form's generated source file will have externs declared for any image that is needed. These are no longer generated in the header file, since the file containing the image definition could change any time the user changes form position.
Images List file
The wxueImage() and wxueBundleSVG() functions have been removed from this file. In their place are get_image() and get_bundle_svg() which are within the wxue_img namespace. Normally, these will not be used by a dev, since there are both bundle_...() and image_...() functions for loading any image added to the list that can be loaded into a wxBitmapBundle or wxImage.
By default, the header file only contains the namespace and all of the available functions. A add_extern property has been added to the Images List node -- and if checked, extern declarations will be added to the header file after all of the function declarations.
wxWidgets 3.1 code generation
This is still broken around images, and this PR probably makes it worse. A second branch is active with 3.1 code generation fixes, and after this PR is merged into it, work will continue on that branch. Chances are, if you mark your project as using wxWidgets 3.1, you will not be able to compile it.