Respond to comments

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Author: steffenlarsen

sycl/source/detail/global_handler.cpp

@@ -118,7 +118,7 @@ void GlobalHandler::registerDefaultContextReleaseHandler() {
 
 // Note: Split from shutdown so it is available to the unittests for ensuring
 //       that the mock plugin is the lone plugin.
-void GlobalHandler::clearPlugins() {
+void GlobalHandler::unloadPlugins() {
   // Call to GlobalHandler::instance().getPlugins() initializes plugins. If
   // user application has loaded SYCL runtime, and never called any APIs,
   // there's no need to load and unload plugins.
@@ -153,7 +153,7 @@ void shutdown() {
   GlobalHandler::instance().MProgramManager.Inst.reset(nullptr);
 
   // Clear the plugins and reset the instance if it was there.
-  GlobalHandler::instance().clearPlugins();
+  GlobalHandler::instance().unloadPlugins();
   if (GlobalHandler::instance().MPlugins.Inst)
     GlobalHandler::instance().MPlugins.Inst.reset(nullptr);
 

sycl/source/detail/global_handler.hpp

@@ -71,7 +71,7 @@ class GlobalHandler {
 
   static void registerDefaultContextReleaseHandler();
 
-  void clearPlugins();
+  void unloadPlugins();
 
 private:
   friend void releaseDefaultContexts();

sycl/source/detail/posix_pi.cpp

@@ -30,6 +30,8 @@ void *loadOsLibrary(const std::string &PluginPath) {
 }
 
 int unloadOsLibrary(void *Library) {
+  // The mock plugin does not have an associated library, so we allow nullptr
+  // here to avoid it trying to free a non-existent library.
   if (!Library)
     return 0;
   return dlclose(Library);

sycl/source/detail/windows_pi.cpp

@@ -41,6 +41,8 @@ void *loadOsLibrary(const std::string &PluginPath) {
 }
 
 int unloadOsLibrary(void *Library) {
+  // The mock plugin does not have an associated library, so we allow nullptr
+  // here to avoid it trying to free a non-existent library.
   if (!Library)
     return 1;
   return (int)FreeLibrary((HMODULE)Library);

sycl/unittests/assert/assert.cpp

@@ -527,6 +527,9 @@ void ParentProcess(int ChildPID, int ChildStdErrFD) {
 #endif // _WIN32
 
 TEST(Assert, TestPositive) {
+  // Ensure that the mock plugin is initialized before spawning work. Since the
+  // test needs no redefinitions we do not need to create a PiMock instance, but
+  // the mock plugin is still needed to have a valid platform available.
   sycl::unittest::PiMock::EnsureMockPluginInitialized();
 
 #ifndef _WIN32

sycl/unittests/helpers/PiMock.hpp

@@ -187,7 +187,7 @@ class PiMock {
     // Ensure that the other plugins are initialized so we can unload them.
     // This makes sure that the mock plugin is the only available plugin.
     detail::pi::initialize();
-    detail::GlobalHandler::instance().clearPlugins();
+    detail::GlobalHandler::instance().unloadPlugins();
     std::vector<detail::plugin> &Plugins =
         detail::GlobalHandler::instance().getPlugins();
 

sycl/unittests/kernel-and-program/Cache.cpp

@@ -91,15 +91,6 @@ struct TestCtx {
 
 std::unique_ptr<TestCtx> globalCtx;
 
-static pi_result redefinedProgramCreateWithSource(pi_context context,
-                                                  pi_uint32 count,
-                                                  const char **strings,
-                                                  const size_t *lengths,
-                                                  pi_program *ret_program) {
-  *ret_program = reinterpret_cast<pi_program>(1);
-  return PI_SUCCESS;
-}
-
 static pi_result redefinedKernelGetInfo(pi_kernel kernel,
                                         pi_kernel_info param_name,
                                         size_t param_value_size,
@@ -113,25 +104,13 @@ static pi_result redefinedKernelGetInfo(pi_kernel kernel,
   return PI_SUCCESS;
 }
 
-static pi_result redefinedKernelCreate(pi_program program,
-                                       const char *kernel_name,
-                                       pi_kernel *ret_kernel) {
-  return PI_SUCCESS;
-}
-
-static pi_result redefinedKernelRelease(pi_kernel kernel) { return PI_SUCCESS; }
-
 class KernelAndProgramCacheTest : public ::testing::Test {
 public:
   KernelAndProgramCacheTest() : Mock{}, Plt{Mock.getPlatform()} {}
 
 protected:
   void SetUp() override {
-    Mock.redefine<detail::PiApiKind::piclProgramCreateWithSource>(
-        redefinedProgramCreateWithSource);
     Mock.redefine<detail::PiApiKind::piKernelGetInfo>(redefinedKernelGetInfo);
-    Mock.redefine<detail::PiApiKind::piKernelCreate>(redefinedKernelCreate);
-    Mock.redefine<detail::PiApiKind::piKernelRelease>(redefinedKernelRelease);
   }
 
 protected:

sycl/unittests/scheduler/QueueFlushing.cpp

@@ -140,10 +140,9 @@ TEST_F(SchedulerTest, QueueFlushing) {
       redefinedEnqueueMemBufferFill);
 
   context Ctx{Plt};
-  default_selector Selector;
-  queue QueueA{Ctx, Selector};
+  queue QueueA{Ctx, default_selector_v};
   detail::QueueImplPtr QueueImplA = detail::getSyclObjImpl(QueueA);
-  queue QueueB{Ctx, Selector};
+  queue QueueB{Ctx, default_selector_v};
   detail::QueueImplPtr QueueImplB = detail::getSyclObjImpl(QueueB);
   ExpectedDepQueue = QueueImplB->getHandleRef();
 
@@ -214,7 +213,7 @@ TEST_F(SchedulerTest, QueueFlushing) {
                                 access::mode::read_write};
     detail::EventImplPtr DepEvent;
     {
-      queue TempQueue{Ctx, Selector};
+      queue TempQueue{Ctx, default_selector_v};
       detail::QueueImplPtr TempQueueImpl = detail::getSyclObjImpl(TempQueue);
       DepEvent.reset(new detail::event_impl(TempQueueImpl));
       DepEvent->setContextImpl(TempQueueImpl->getContextImplPtr());