Skip to content

Conversation

swebb2066
Copy link
Contributor

@swebb2066 swebb2066 commented Aug 21, 2022

This commit is progress toward resolving LOGCXX-558

@swebb2066 swebb2066 changed the title Prevent MSVC 'nedds to have a dll interface' warnings Prevent MSVC 'needs to have a dll interface' warnings Aug 21, 2022
@swebb2066 swebb2066 marked this pull request as ready for review August 22, 2022 00:45
@swebb2066 swebb2066 requested a review from rm5248 August 22, 2022 00:45
@@ -64,8 +64,15 @@ typedef unsigned int log4cxx_uint32_t;
typedef std::weak_ptr<T> T##WeakPtr
#define LOG4CXX_UNIQUE_PTR_DEF(T) typedef std::unique_ptr<T> T##UniquePtr;
#define LOG4CXX_LIST_DEF(N, T) typedef std::vector<T> N
#define LOG4CXX_PRIVATE_PTR(T) std::unique_ptr<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this define really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only wanted the definitions to be in the same place, so the usage is (for example)
LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(AppenderSkeletonPrivate, m_priv)
AppenderSkeleton(LOG4CXX_PRIVATE_PTR(AppenderSkeletonPrivate) priv);

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like it doesn't really add anything in terms of making things simpler to read(to me).

Not really a huge issue, more of a preference thing in how to organize the code.

#else
#define LOG4CXX_EXPORT
#define LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(T, V) struct T; LOG4CXX_PRIVATE_PTR(T) V;
#define LOG4CXX_INSTANTIATE_EXPORTED_PTR(T)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have two versions of this macro? I don't know enough about how windows works, but would it be possible to just add LOG4CXX_EXPORT to the already-existing LOG4CXX_PTR_DEF macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gcc says instantiated templates must be in their own namespace (not sure how to interpret that when the template and the template parameter are in different namespaces). Visual Studio does not issue that error, but not instantiating the template at all results in a 'needs to have a dll interface' warning.

My solution was to only instantiate when using Visual Studio.

See for example, in propertysetter.h

Copy link
Contributor

@rm5248 rm5248 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, I'm just a little confused as to the macro definitions.

Please squash your commits when merging.

@swebb2066 swebb2066 merged commit d74edc6 into apache:next_stable Sep 3, 2022
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 this pull request may close these issues.

3 participants