[cmake-developers] _PyPopenProcs warning and comment in cmWin32ProcessExecution.cxx

[David Cole]

Yes. That's clearly from the original source of this file.... (It was
copied into CMake from ITK, and looks like it must have come from
python originally.)

The comments are completely irrelevant to the existing code from today.


On Thu, Nov 8, 2012 at 1:43 PM, Stephen Kelly <steveire at gmail.com> wrote:
> Brad King wrote:
>
>> On 11/08/2012 12:44 PM, Stephen Kelly wrote:
>>>
>>> Hi there,
>>>
>>> http://open.cdash.org/viewBuildError.php?type=1&buildid=2652291
>>>
>>> has a new warning (as a result of my adding warning flags) of an unused
>>> variable. The variable is referenced in a comment. I'm guessing a snippet
>>> came from elsewhere.
>>>
>>> I can remove the _PyPopenProcs variable, but I think the comment should
>>> be fixed too at the same time. However, I don't know what the comment
>>> should be changed to.
>>
>> That is an ancient version of the process execution implementation
>> which is only kept for backward compatibility to implement the
>> exec_program() command IIRC.  We don't maintain it anymore so the
>> comments don't matter.  Just remove the unused variable and its
>> comment.
>
> If I remove all comments which refer to a _Py* variable, it looks like this:
>
> diff --git a/Source/cmWin32ProcessExecution.cxx
> b/Source/cmWin32ProcessExecution.cxx
> index 9a536c1..5752ab6 100644
> --- a/Source/cmWin32ProcessExecution.cxx
> +++ b/Source/cmWin32ProcessExecution.cxx
> @@ -271,13 +271,6 @@ bool cmWin32ProcessExecution::Wait(int timeout)
>    return this->PrivateClose(timeout);
>  }
>
> -/*
> - * Internal dictionary mapping popen* file pointers to process handles,
> - * for use when retrieving the process exit code.  See _PyPclose() below
> - * for more information on this dictionary's use.
> - */
> -static void *_PyPopenProcs = NULL;
> -
>  static BOOL RealPopenCreateProcess(const char *cmdstring,
>                                     const char *path,
>                                     const char *szConsoleSpawn,
> @@ -679,18 +672,6 @@ bool cmWin32ProcessExecution::PrivateOpen(const char
> *cmdstring,
>        }
>      }
>
> -  /*
> -   * Insert the files we've created into the process dictionary
> -   * all referencing the list with the process handle and the
> -   * initial number of files (see description below in _PyPclose).
> -   * Since if _PyPclose later tried to wait on a process when all
> -   * handles weren't closed, it could create a deadlock with the
> -   * child, we spend some energy here to try to ensure that we
> -   * either insert all file handles into the dictionary or none
> -   * at all.  It's a little clumsy with the various popen modes
> -   * and variable number of files involved.
> -   */
> -
>    /* Child is launched. Close the parents copy of those pipe
>     * handles that only the child should have open.  You need to
>     * make sure that no handles to the write end of the output pipe
> @@ -761,43 +742,6 @@ cmWin32ProcessExecution::~cmWin32ProcessExecution()
>    this->CloseHandles();
>  }
>
> -/*
> - * Wrapper for fclose() to use for popen* files, so we can retrieve the
> - * exit code for the child process and return as a result of the close.
> - *
> - * This function uses the _PyPopenProcs dictionary in order to map the
> - * input file pointer to information about the process that was
> - * originally created by the popen* call that created the file pointer.
> - * The dictionary uses the file pointer as a key (with one entry
> - * inserted for each file returned by the original popen* call) and a
> - * single list object as the value for all files from a single call.
> - * The list object contains the Win32 process handle at [0], and a file
> - * count at [1], which is initialized to the total number of file
> - * handles using that list.
> - *
> - * This function closes whichever handle it is passed, and decrements
> - * the file count in the dictionary for the process handle pointed to
> - * by this file.  On the last close (when the file count reaches zero),
> - * this function will wait for the child process and then return its
> - * exit code as the result of the close() operation.  This permits the
> - * files to be closed in any order - it is always the close() of the
> - * final handle that will return the exit code.
> - */
> -
> - /* RED_FLAG 31-Aug-2000 Tim
> -  * This is always called (today!) between a pair of
> -  * Py_BEGIN_ALLOW_THREADS/ Py_END_ALLOW_THREADS
> -  * macros.  So the thread running this has no valid thread state, as
> -  * far as Python is concerned.  However, this calls some Python API
> -  * functions that cannot be called safely without a valid thread
> -  * state, in particular PyDict_GetItem.
> -  * As a temporary hack (although it may last for years ...), we
> -  * *rely* on not having a valid thread state in this function, in
> -  * order to create our own "from scratch".
> -  * This will deadlock if _PyPclose is ever called by a thread
> -  * holding the global lock.
> -  */
> -
>  bool cmWin32ProcessExecution::PrivateClose(int /* timeout */)
>  {
>    HANDLE hProcess = this->ProcessHandle;
>
>
>
>
> Should I commit that?
>
> Thanks,
>
> Steve.
>
>
> --
>
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>
> Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers