You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/09/19 08:17:41 UTC
[GitHub] [logging-log4cxx] swebb2066 opened a new pull request, #132: Use std::make_shared to create std::shared_ptr instances
swebb2066 opened a new pull request, #132:
URL: https://github.com/apache/logging-log4cxx/pull/132
This is a performance improvement as std::make_shared makes only one allocation.
Some previously private constructors have been made public as std::make_shared needs access to the constructor. While these constructors are not intended for client use, [alternative](https://stackoverflow.com/questions/8147027/how-do-i-call-stdmake-shared-on-a-class-with-only-protected-or-private-const) approaches seem unnessessaryly complex.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 merged pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
swebb2066 merged PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132#discussion_r974769603
##########
src/main/include/log4cxx/hierarchy.h:
##########
@@ -66,15 +66,10 @@ class LOG4CXX_EXPORT Hierarchy :
LOG4CXX_CAST_ENTRY(spi::LoggerRepository)
END_LOG4CXX_CAST_MAP()
- private:
- /**
- Create a new logger hierarchy.
- */
- Hierarchy();
-
public:
static HierarchyPtr create();
+ Hierarchy();
Review Comment:
Then I should also change
`LoggerRepositoryPtr Logger::getLoggerRepository () const`
back to
`LoggerRepository* Logger::getLoggerRepository () const`
as it is in 0.13 - OK?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132#discussion_r974276552
##########
src/main/include/log4cxx/helpers/threadutility.h:
##########
@@ -69,15 +69,14 @@ LOG4CXX_PTR_DEF(ThreadUtility);
class LOG4CXX_EXPORT ThreadUtility
{
private:
- ThreadUtility();
-
log4cxx::helpers::ThreadStartPre preStartFunction();
log4cxx::helpers::ThreadStarted threadStartedFunction();
log4cxx::helpers::ThreadStartPost postStartFunction();
LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(priv_data, m_priv)
public:
+ ThreadUtility();
Review Comment:
It seems like this would break the singleton aspect of this class, do we really want it to have a public constructor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132#discussion_r974766904
##########
src/main/include/log4cxx/helpers/threadutility.h:
##########
@@ -69,15 +69,14 @@ LOG4CXX_PTR_DEF(ThreadUtility);
class LOG4CXX_EXPORT ThreadUtility
{
private:
- ThreadUtility();
-
log4cxx::helpers::ThreadStartPre preStartFunction();
log4cxx::helpers::ThreadStarted threadStartedFunction();
log4cxx::helpers::ThreadStartPost postStartFunction();
LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(priv_data, m_priv)
public:
+ ThreadUtility();
Review Comment:
Should I change
`std::shared_ptr<ThreadUtility> ThreadUtility::instance()`
to
`ThreadUtility& ThreadUtility::instance()`
or
`ThreadUtility* ThreadUtility::instance()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132#discussion_r974279067
##########
src/main/include/log4cxx/hierarchy.h:
##########
@@ -66,15 +66,10 @@ class LOG4CXX_EXPORT Hierarchy :
LOG4CXX_CAST_ENTRY(spi::LoggerRepository)
END_LOG4CXX_CAST_MAP()
- private:
- /**
- Create a new logger hierarchy.
- */
- Hierarchy();
-
public:
static HierarchyPtr create();
+ Hierarchy();
Review Comment:
Since there's already a `create` method, this doesn't need to be public - I think this should be a singleton anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132#discussion_r974816672
##########
src/main/include/log4cxx/hierarchy.h:
##########
@@ -66,15 +66,10 @@ class LOG4CXX_EXPORT Hierarchy :
LOG4CXX_CAST_ENTRY(spi::LoggerRepository)
END_LOG4CXX_CAST_MAP()
- private:
- /**
- Create a new logger hierarchy.
- */
- Hierarchy();
-
public:
static HierarchyPtr create();
+ Hierarchy();
Review Comment:
That seems reasonable, as long as it is a singleton(I'm fairly certain that it is).
Would changing it from a `shared_ptr` to a raw pointer improve performance at all? I know it did for some other changes you had done before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #132: Use std::make_shared to create std::shared_ptr instances
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #132:
URL: https://github.com/apache/logging-log4cxx/pull/132#discussion_r974812641
##########
src/main/include/log4cxx/helpers/threadutility.h:
##########
@@ -69,15 +69,14 @@ LOG4CXX_PTR_DEF(ThreadUtility);
class LOG4CXX_EXPORT ThreadUtility
{
private:
- ThreadUtility();
-
log4cxx::helpers::ThreadStartPre preStartFunction();
log4cxx::helpers::ThreadStarted threadStartedFunction();
log4cxx::helpers::ThreadStartPost postStartFunction();
LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(priv_data, m_priv)
public:
+ ThreadUtility();
Review Comment:
Keeping that as a `shared_ptr` makes it similar to other methods(for consistency), but since it is a singleton making it a `ThreadUtility*` would be fine in my opinion as well, since it is guaranteed to never be null in normal operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org