search

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

Small code modernization changes. #16

Closed nshcat closed 8 years ago

nshcat commented 8 years ago

I suggest replacing the C99 VLA usage with a dynamic buffer (as done in commit 7eda337 ) since there are still important C++ compilers who do not seem to support C99 (like some versions of MSVC/Visual Studio).

I also replaced all occurrences of operator new since raw usage of it is discouraged; std::make_unique<> does the job just fine in this case.

Finally, I replaced C-Style NULL macro usage in WinAPI calls with modern C++'s nullptr.

Feel free to cherry pick if you do not like the VLA change.

eidheim commented 8 years ago

Thank you again. My only worry is that we might be a bit premature in moving to C++14 (std::make_unique) for a library such as this. Many are still using Ubuntu 14 on their servers, even Travis CI has said they will not move to Ubuntu 16 before next year. What are your thoughts on this?

nshcat commented 8 years ago

I honestly wasn't aware that the repositories of popular distributions are lagging that far behind in terms of compiler versions (I am running Arch Linux for testing stuff, I never tinkered with Ubuntu or Debian). So, of course, this doesn't sound like that great of an idea anymore. I should've researched this a little bit more before doing the request.

Still, I would replace the VLA usage to support VisualStudio 2013 and friends. It obviously can be done without using make_unique . I will look into this.

eidheim commented 8 years ago

Ubuntu made things worse sadly, by lagging behind debian stable. Debian stable actually supports C++14.

However, I can push the https://github.com/eidheim/tiny-process-library/pull/16/commits/7eda3373701cdbd5724910083f8b791d8461ecb7 (with std::make_unique replaced with std::unique_ptr) and https://github.com/eidheim/tiny-process-library/pull/16/commits/e1ee371d03b070b6e8b8be7c81e4153ff12a0820 commits now, and we could maybe wait a bit with the C++14 features.

eidheim commented 8 years ago

Thank you, saw your edit now, I'll wait for your changes.

nshcat commented 8 years ago

I pushed my changes.

Sorry for the inconvenience.

eidheim commented 8 years ago

No worries, and many thanks for looking through the source code.