eidheim / tiny-process-library

A small platform independent library making it simple to create and stop new processes in C++, as well as writing to stdin and reading from stdout and stderr of a new process
MIT License
338 stars 73 forks source link

Create a handle wrapper to avoid calling CloseHandle() several times #7

Closed dacap closed 8 years ago

dacap commented 8 years ago

Something similar could be done for file descriptors in the unix port (to avoid calling close(...)).

eidheim commented 8 years ago

An alternative way of doing this could be the following Handle class:

class Handle : public std::unique_ptr<HANDLE, std::function<void(HANDLE *)> > {
public:
  Handle() : std::unique_ptr<HANDLE, std::function<void(HANDLE *)> >(new HANDLE(NULL), [](HANDLE *handle) {
    if(handle!=NULL)
      CloseHandle(handle);
  }) {}
};

And make the appropriate changes in process_win.cpp, that is using Handle::release, Handle::get, and keep the if statements with INVALID_HANDLE_VALUE in Process::open.

dacap commented 8 years ago

An alternative way

I don't think so. What does new HANDLE(NULL) means? why would we need to create a handle to nullptr if it isn't even used? When an invalid handle has INVALID_HANDLE_VALUE value? (So get() returns INVALID_HANDLE_VALUE instead of null)

Also I'm not sure if it's "legal" (recommended) subclassing std::unique_ptr. Should not the Deleter template parameter be CloseHandle directly (or other function)? (e.g. instead of subclassing I think that we should use something like a typedef?

Anyway, fixing all this, I would prefer a direct approach instead of adapting std::unique_ptr because the code is more readable.

eidheim commented 8 years ago

You are right, it's more clean the way you do it.