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

Fix QVariantMap linker error #2679

Closed
wants to merge 1 commit into from
Closed

Conversation

@mitchcurtis
Copy link
Contributor

mitchcurtis commented Nov 23, 2019

https://wiki.qt.io/Coding_Conventions says not to derive from template/tool classes.

The error message was:

tiled.lib(tiled.dll) : error LNK2005: "public: __cdecl QMap<class QString,class QVariant>::~QMap<class QString,class QVariant>(void)" (??1?$QMap@VQString@@VQVariant@@@@QEAA@XZ) already defined in SpriteSequence.cpp.obj
Creating library C:\dev\tshnm-iso-qt5_14_d-Debug\Debug\libisle.7c81f238\isle.lib and object C:\dev\tshnm-iso-qt5_14_d-Debug\Debug\libisle.7c81f238\isle.exp
C:\dev\tshnm-iso-qt5_14_d-Debug\Debug\libisle.7c81f238\isle.dll : fatal error LNK1169: one or more multiply defined symbols found

https://wiki.qt.io/Coding_Conventions says not to derive from template/tool classes.

The error message was:

tiled.lib(tiled.dll) : error LNK2005: "public: __cdecl QMap<class QString,class QVariant>::~QMap<class QString,class QVariant>(void)" (??1?$QMap@VQString@@VQVariant@@@@QEAA@XZ) already defined in SpriteSequence.cpp.obj
   Creating library C:\dev\tshnm-iso-qt5_14_d-Debug\Debug\libisle.7c81f238\isle.lib and object C:\dev\tshnm-iso-qt5_14_d-Debug\Debug\libisle.7c81f238\isle.exp
C:\dev\tshnm-iso-qt5_14_d-Debug\Debug\libisle.7c81f238\isle.dll : fatal error LNK1169: one or more multiply defined symbols found
@mitchcurtis

This comment has been minimized.

Copy link
Contributor Author

mitchcurtis commented Nov 23, 2019

Note that I didn't test anything other than libtiled and tiledquickplugin as I no longer build all the other stuff.

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Nov 25, 2019

Note that I didn't test anything other than libtiled and tiledquickplugin as I no longer build all the other stuff.

Yeah, all checks are failing. :-)

I would propose not to do this by composition since too many other parts of the code need to be changed (and duplicating the entire QMap API is rather annoying). Instead, my proposal would be to do:

using Properties = QVariantMap;

And then defining the few extra members that the Properties subclass had as stand-alone functions. Something like:

TILEDSHARED_EXPORT void mergeProperties(Properties &into, const Properties &from);
TILEDSHARED_EXPORT QJsonArray propertiesToJson(const Properties &properties);
TILEDSHARED_EXPORT Properties propertiesFromJson(const QJsonArray &json);

And for consistency I think we should do the same with AggregatedProperties.

@mitchcurtis

This comment has been minimized.

Copy link
Contributor Author

mitchcurtis commented Nov 25, 2019

Yeah, all checks are failing. :-)

You've gotta break some eggs to make an omelette! :D

And for consistency I think we should do the same with AggregatedProperties.

How would that work? It doesn't derive from QVariantMap; it uses: QMap<QString, AggregatedPropertyData>

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Nov 25, 2019

How would that work? It doesn't derive from QVariantMap; it uses: QMap<QString, AggregatedPropertyData>

Yes, but it also derives from it, which is the problem. If any other lib used libtiled and needed to destroy a QMap<QString, AggregatedPropertyData>, it would probably run into the same symbol clashes on its destructor. This case would be pretty rare, hence I suggested to replace that class with using AggregatedProperties = QMap<QString, AggregatedPropertyData> for consistency only.

@bjorn bjorn closed this in 19032f5 Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.