Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed usage of Windows.h #7

Closed

Conversation

klemens-morgenstern
Copy link

So I want to use the library and bloody hate the win-api, hence i removed the usage of it and build in substitutes.

The thing compiles with MSVC 2015 and Mingw 5.1 but not all tests pass. The problem here is, that I do not know the library well enough to debug this. Additionally, I'd need to know if you'd be even interested in this change, since I'd guess we'd also have to provide tests for the winapi-stuff or may have more work to comply to the boost conventions.

So if you've got the time, I'd be really glad if you could look over the code.

Hopefuly this closes #4.

@klemens-morgenstern
Copy link
Author

I will try to move the changes to boostorg/winapi, and as soon as this is accepted rework this pull request to make more use of this.
However, as an experimental implementation it's quite useful already.

@BorisSchaeling
Copy link
Owner

Thanks for your patch! I agree with you that boostorg/winapi is a better place though. Current and future Boost.Process maintainers can then concentrate on process management only (which is complicated enough :) without having to dive into additional details of the Windows API.

This is because of the current attempt to pull the winapi stuff into the boost/winapi repo. Here's the [pull request](boostorg/winapi#16)
@klemens-morgenstern
Copy link
Author

Currently the pull request is pending. If that is accepted we could pull - that might however need to be on the develop branch, since we need the develop branch of develop

@klemens-morgenstern
Copy link
Author

Ok, it's officially broken, I'll work on that, and tell you as soon as fixed.

@BorisSchaeling
Copy link
Owner

Sounds good! - Do I understand correctly that you recommend us creating a develop branch?

@klemens-morgenstern
Copy link
Author

@BorisSchaeling Currently, windows provides two versions for the functions, which may either take wchar_t or char_t. Now I changed boost::filesystem::path to be converted to wchar_t by default, because wchar_t is the default representation on windows. Since the executor also uses the wchar_t types currently (aea7337) , the tests fail.

I would propose the following:

Make executor into a template, and defined typedefs in the following way:

template<typename CharType> struct basic_executor;

typedef basic_executor<char>         executor;
typedef basic_executor<wchar_t> wexecutor;

The other variant would be to just make everything use wchar_t and just convert everything into wchar_t.

Edit: I forgot to mention: I changed the underlying classes to be available if they are available on the platorm. From what I can tell, the WCHAR version is always available, while the CHAR might not be (which afaik is a rather obscure case).

I would then also look for the parameters in execute, whether one of them needs wchar_t, and thereby select if it'll use executor or wexecutor. If this double implementation is not needed, I think I'd use the WCHAR variant, as boost::filesystem::path does.

@klemens-morgenstern
Copy link
Author

Well, I am just saying: until the winapi changes go into the master branch will take a while. It would be strange to have process/master dependent on winapi/develop.

But that's not relevant for travis, since it only builds the linux version.

@klemens-morgenstern
Copy link
Author

Ok, this branch could actually be merged, if the user considers, that boost::filesystem::pathwill be converted to an std::wstring. I am rather hesitant to implement an auto-conversion, since this would require me to rework the execute function,

@klemens-morgenstern
Copy link
Author

I would consider this PR obsolete since the development of the new version of boost.process seems to lead somewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid inclusion of <windows.h>
3 participants