Replace ExpandEnvironmentStrings double calling with wil helper #3198
Conversation
src/host/ft_host/CanaryTests.cpp
Outdated
| @@ -67,7 +65,7 @@ void CanaryTests::LaunchV1Console() | |||
|
|
|||
| wil::unique_process_information ProcessInformation; | |||
| VERIFY_WIN32_BOOL_SUCCEEDED(CreateProcessW(NULL, | |||
| CmdLineMutable.get(), | |||
| const_cast<LPWSTR>(CmdLine.c_str()), | |||
There was a problem hiding this comment.
So this one should definitely use .data() -- CreateProcessW requires a mutable string (!) and since data is the mutable variety of c_str, it's a better fit
There was a problem hiding this comment.
A mutable string, huh. That's not what I'd expected. You learn something everyday.
src/server/Entrypoints.cpp
Outdated
| // Call create process | ||
| wil::unique_process_information ProcessInformation; | ||
| RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW(NULL, | ||
| CmdLineMutable.get(), | ||
| const_cast<LPWSTR>(cmdLine.c_str()), |
There was a problem hiding this comment.
and, bonus, no need for a const_cast!
ghost
added
Needs-Author-Feedback
skyline75489
force-pushed
the
fix/2097
branch
from
9808131
to
b823d61
Compare
There was a problem hiding this comment.
Love it! One note: I just realized that you use rebase and force push. I love that too, however(!), it doesn't generate an e-mail when you submit a new iteration. Would you mind commenting to let us know that you've got another iteration ready?
I know I've missed a couple of your pull requests because of that 😄
|
@DHowett-MSFT I didn't realize force-push doesn't generate email notification till now. Thanks for the heads up |
| // Call create process | ||
| wil::unique_process_information ProcessInformation; | ||
| RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW(NULL, | ||
| CmdLineMutable.get(), | ||
| cmdLine.data(), |
There was a problem hiding this comment.
By the way, should this be capital or not? In the aspect of modern C++, I'd say cmdLine is better.
There was a problem hiding this comment.
Personally, I like cmdLine more
| // Call create process | ||
| wil::unique_process_information ProcessInformation; | ||
| RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW(NULL, | ||
| CmdLineMutable.get(), | ||
| cmdLine.data(), |
There was a problem hiding this comment.
Personally, I like cmdLine more
ghost
requested review from
DHowett-MSFT
changed the title
|
Thanks for the contribution! |
|
🎉 Handy links: |
ghost
mentioned this pull request
Summary of the Pull Request
References
#2097
PR Checklist
Closes use wil::ExpandEnvironmentStrings where possible #2097
CLA signed. If not, go over here and sign the CLA
Tests added/passedRequires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed