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 2021/12/29 01:54:31 UTC
[GitHub] [logging-log4cxx] swebb2066 opened a new pull request #93: Add a unit test for the automatic configuration feature
swebb2066 opened a new pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93
This new unit test covers the undocumented automatic configuration function.
--
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 change in pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#discussion_r776895762
##########
File path: src/main/cpp/aprinitializer.cpp
##########
@@ -127,3 +127,21 @@ void APRInitializer::unregisterCleanup(FileWatchdog* watchdog)
}
}
+void APRInitializer::addObject(size_t key, const ObjectPtr& pObject)
+{
+#if APR_HAS_THREADS
+ std::unique_lock<std::mutex> lock(this->mutex);
+#endif
+ this->objects[key] = pObject;
+}
+
+const ObjectPtr& APRInitializer::findOrAddObject(size_t key, std::function<ObjectPtr()> creator)
+{
+#if APR_HAS_THREADS
+ std::unique_lock<std::mutex> lock(this->mutex);
+#endif
+ auto pItem = this->objects.find(key);
+ if (this->objects.end() == pItem)
+ pItem = this->objects.emplace(key, creator()).first;
Review comment:
We should probably check to make sure that the `creator` function is in fact valid first.
##########
File path: src/main/cpp/logmanager.cpp
##########
@@ -47,28 +47,19 @@ using namespace log4cxx::helpers;
IMPLEMENT_LOG4CXX_OBJECT(DefaultRepositorySelector)
void* LogManager::guard = 0;
-spi::RepositorySelectorPtr LogManager::repositorySelector;
-
RepositorySelectorPtr LogManager::getRepositorySelector()
{
- //
- // call to initialize APR and trigger "start" of logging clock
- //
- APRInitializer::initialize();
-
- if (!repositorySelector)
- {
- LoggerRepositoryPtr hierarchy = Hierarchy::create();
- RepositorySelectorPtr selector(new DefaultRepositorySelector(hierarchy));
- repositorySelector = selector;
- }
-
- return repositorySelector;
+ auto result = APRInitializer::getUnique<spi::RepositorySelector>( []() -> ObjectPtr
+ {
+ LoggerRepositoryPtr hierarchy = Hierarchy::create();
+ return ObjectPtr(new DefaultRepositorySelector(hierarchy));
+ }
+ );
Review comment:
This kinda broke my brain here. If it is called `getUnique`, I would expect it to only get something, not try to also construct something. The private method name(`findOrAddObject`) is much better, although perhaps the signature would make more sense as `getOrCreate<T>( std::function<T> create_fn )`? It makes more sense to me to have the function be a parameter so that you know that you may or may not call it, but I don't know if other people feel the same way.
##########
File path: src/main/cpp/logmanager.cpp
##########
@@ -47,28 +47,19 @@ using namespace log4cxx::helpers;
IMPLEMENT_LOG4CXX_OBJECT(DefaultRepositorySelector)
void* LogManager::guard = 0;
-spi::RepositorySelectorPtr LogManager::repositorySelector;
-
RepositorySelectorPtr LogManager::getRepositorySelector()
{
- //
- // call to initialize APR and trigger "start" of logging clock
- //
- APRInitializer::initialize();
-
- if (!repositorySelector)
- {
- LoggerRepositoryPtr hierarchy = Hierarchy::create();
- RepositorySelectorPtr selector(new DefaultRepositorySelector(hierarchy));
- repositorySelector = selector;
- }
-
- return repositorySelector;
+ auto result = APRInitializer::getUnique<spi::RepositorySelector>( []() -> ObjectPtr
+ {
+ LoggerRepositoryPtr hierarchy = Hierarchy::create();
+ return ObjectPtr(new DefaultRepositorySelector(hierarchy));
+ }
+ );
Review comment:
upon further looking at it the signature is a function, but that name is still confusing to me.
--
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 pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
rm5248 commented on pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#issuecomment-1003560920
Apparently I forgot to submit my review comments, but those are above.
--
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 change in pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#discussion_r777154247
##########
File path: src/main/cpp/aprinitializer.cpp
##########
@@ -127,3 +127,21 @@ void APRInitializer::unregisterCleanup(FileWatchdog* watchdog)
}
}
+void APRInitializer::addObject(size_t key, const ObjectPtr& pObject)
+{
+#if APR_HAS_THREADS
+ std::unique_lock<std::mutex> lock(this->mutex);
+#endif
+ this->objects[key] = pObject;
+}
+
+const ObjectPtr& APRInitializer::findOrAddObject(size_t key, std::function<ObjectPtr()> creator)
+{
+#if APR_HAS_THREADS
+ std::unique_lock<std::mutex> lock(this->mutex);
+#endif
+ auto pItem = this->objects.find(key);
+ if (this->objects.end() == pItem)
+ pItem = this->objects.emplace(key, creator()).first;
Review comment:
Depending on what it is used for, if I want to get an object of some kind from here there is currently no way to do that, we would have to either pass in a an invalid function, or create a new method `getObject<T>`, or something like that. I'm not sure if we would ever want to do that.
As it is, it isn't really "undefined behavior," since it would throw `std::bad_function_call` if an invalid function was passed. We could just change `if (this->objects.end() == pItem)` to `if (this->objects.end() == pItem && creator)` since `operator bool()` is provided.
--
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 change in pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on a change in pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#discussion_r777150208
##########
File path: src/main/cpp/aprinitializer.cpp
##########
@@ -127,3 +127,21 @@ void APRInitializer::unregisterCleanup(FileWatchdog* watchdog)
}
}
+void APRInitializer::addObject(size_t key, const ObjectPtr& pObject)
+{
+#if APR_HAS_THREADS
+ std::unique_lock<std::mutex> lock(this->mutex);
+#endif
+ this->objects[key] = pObject;
+}
+
+const ObjectPtr& APRInitializer::findOrAddObject(size_t key, std::function<ObjectPtr()> creator)
+{
+#if APR_HAS_THREADS
+ std::unique_lock<std::mutex> lock(this->mutex);
+#endif
+ auto pItem = this->objects.find(key);
+ if (this->objects.end() == pItem)
+ pItem = this->objects.emplace(key, creator()).first;
Review comment:
As getRepositorySelector() is assumed in all calls to always succeed, an invalid `creator` is 'undefined behaviour'.
Also, the `creator ` function is an object, not a pointer so it has to be provided (it should be a precondition).
--
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 change in pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on a change in pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#discussion_r777150343
##########
File path: src/main/cpp/logmanager.cpp
##########
@@ -47,28 +47,19 @@ using namespace log4cxx::helpers;
IMPLEMENT_LOG4CXX_OBJECT(DefaultRepositorySelector)
void* LogManager::guard = 0;
-spi::RepositorySelectorPtr LogManager::repositorySelector;
-
RepositorySelectorPtr LogManager::getRepositorySelector()
{
- //
- // call to initialize APR and trigger "start" of logging clock
- //
- APRInitializer::initialize();
-
- if (!repositorySelector)
- {
- LoggerRepositoryPtr hierarchy = Hierarchy::create();
- RepositorySelectorPtr selector(new DefaultRepositorySelector(hierarchy));
- repositorySelector = selector;
- }
-
- return repositorySelector;
+ auto result = APRInitializer::getUnique<spi::RepositorySelector>( []() -> ObjectPtr
+ {
+ LoggerRepositoryPtr hierarchy = Hierarchy::create();
+ return ObjectPtr(new DefaultRepositorySelector(hierarchy));
+ }
+ );
Review comment:
How about `getOrAddUnique<T>`?
It need to pair somehow with `setUnique<T>`
--
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 pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#issuecomment-1003508384
Merged as 7fedcdbf using https://gitbox.apache.org
--
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 closed pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
swebb2066 closed pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93
--
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 change in pull request #93: Add a unit test for the automatic configuration feature
Posted by GitBox <gi...@apache.org>.
rm5248 commented on a change in pull request #93:
URL: https://github.com/apache/logging-log4cxx/pull/93#discussion_r777154084
##########
File path: src/main/cpp/logmanager.cpp
##########
@@ -47,28 +47,19 @@ using namespace log4cxx::helpers;
IMPLEMENT_LOG4CXX_OBJECT(DefaultRepositorySelector)
void* LogManager::guard = 0;
-spi::RepositorySelectorPtr LogManager::repositorySelector;
-
RepositorySelectorPtr LogManager::getRepositorySelector()
{
- //
- // call to initialize APR and trigger "start" of logging clock
- //
- APRInitializer::initialize();
-
- if (!repositorySelector)
- {
- LoggerRepositoryPtr hierarchy = Hierarchy::create();
- RepositorySelectorPtr selector(new DefaultRepositorySelector(hierarchy));
- repositorySelector = selector;
- }
-
- return repositorySelector;
+ auto result = APRInitializer::getUnique<spi::RepositorySelector>( []() -> ObjectPtr
+ {
+ LoggerRepositoryPtr hierarchy = Hierarchy::create();
+ return ObjectPtr(new DefaultRepositorySelector(hierarchy));
+ }
+ );
Review comment:
`getOrAddUnique<T>` sounds fine to me.
--
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