Added windows unicode support

[clunietp]

Fixed issue with not compiling on unicode project due to CreateProcess WinAPI remapping to wide version.

[eidheim]

Could you give a small test case that is not currently working and this PR fixes?

[clunietp]

I would like to, except the only time it won't compile is when i define Unicode in the project settings in msvc. I'm sure I could dig up the command line compilation switch but I'm not sure that will help. Let me know if there's another way I can get you what you're looking for

On Sun, Apr 17, 2016, 3:59 PM Ole Christian Eidheim < notifications@github.com> wrote:

Could you give a small test case that is not currently working and this PR fixes?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/eidheim/tiny-process-library/pull/9#issuecomment-211102111

[eidheim]

The _UNICODE is something msvc defines right? I think I can simplify this PR now that I understand this. I'll have a look at it Tomorrow.

However, https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx uses #ifdef UNICODE, not _UNICODE?

[clunietp]

Yes UNICODE looks correct. Thanks for looking into this

On Sun, Apr 17, 2016, 4:21 PM Ole Christian Eidheim < notifications@github.com> wrote:

The _UNICODE is something msvc defines right? I think I can simplify this PR now that I understand this. I'll have a look at it Tomorrow.

However, https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx uses #ifdef UNICODE, not _UNICODE?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/eidheim/tiny-process-library/pull/9#issuecomment-211107546

[eidheim]

My current guess is that the declaration in process.hpp should be:

#if defined(_WIN32) && defined(UNICODE)
  Process(const std::wstring &command, const std::wstring &path=std::wstring(),
#else
  Process(const std::string &command, const std::string &path=std::string(),
#endif
          std::function<void(const char *bytes, size_t n)> read_stdout=nullptr,
          std::function<void(const char *bytes, size_t n)> read_stderr=nullptr,
          bool open_stdin=false,
          size_t buffer_size=131072);

With corresponding changes in process_win.cpp. Then one would not need to convert anything.

And by the way, your use of for instance &path[0] instead of creating a char/wchar was a good idea. edit: the lpCurrentDirectory parameter in CreateProcess is actually const.

[eidheim]

Or even better:

#ifdef _WIN32
  Process(const std::basic_string<TCHAR> &command, const std::basic_string<TCHAR> &path=std::basic_string<TCHAR>(),
#else
  Process(const std::string &command, const std::string &path=std::string(),
#endif
          std::function<void(const char *bytes, size_t n)> read_stdout=nullptr,
          std::function<void(const char *bytes, size_t n)> read_stderr=nullptr,
          bool open_stdin=false,
          size_t buffer_size=131072);

Edit: fixed typos

[clunietp]

That looks pretty good. Or even better, define a typedef in the macro and keep the same signature. Untested Example:

ifdef _win32

Using string_type = std::basic_string;

else

Using string_type = std::string;

end if

Process(const string_type &command, const string_type& path=string_type(), ...

Then you can use string_type elsewhere as needed. Just a thought, thanks again!

Tom

On Sun, Apr 17, 2016, 5:01 PM Ole Christian Eidheim < notifications@github.com> wrote:

Or even better:

ifdef(_WIN32)

Process(const std::basic_char &command, const std::basic_char &path=std::wstring(),

else

Process(const std::string &command, const std::string &path=std::string(),

endif

      std::function<void(const char *bytes, size_t n)> read_stdout=nullptr,
      std::function<void(const char *bytes, size_t n)> read_stderr=nullptr,
      bool open_stdin=false,
      size_t buffer_size=131072);

—

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/eidheim/tiny-process-library/pull/9#issuecomment-211116396

[eidheim]

I fixed the issue in the latest commit, and closing therefore this PR. Thank you very much for the feedback.

@marcosbracco would you mind have a look at https://github.com/eidheim/tiny-process-library/commit/245e36b83670c4bcf19b36c245d0e175d57be178 as well? Just to be on the safe, since I'm no Windows programmer.

[clunietp]

Awesome, thank you! -Tom