Handle spaces in start_process arguments on Windows#494
Conversation
The logic upon which this algorithm is based is derived from recommended practices on an MSDN blog. Signed-off-by: Scott K Logan <logans@cottsay.net>
|
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with a couple of comments.
| char_array->buffer_length = new_length; | ||
|
|
There was a problem hiding this comment.
This store is redundant? i believe that line 134 above, rcutils_char_array_expand_as_needed call also uddates the buffer_length with new_length.
| char_array->buffer_length = new_length; |
There was a problem hiding this comment.
It wouldn't change the length, no. The call to _expand_as_needed will update the capacity if necessary, but the length only gets modified if you shrink the array.
There was a problem hiding this comment.
ah i see. in that case, can we just call rcutils_char_array_resize instead of rcutils_char_array_expand_as_needed to allocate what we need here? i think that updates the buffer_length too. This change ensures that the buffer is resized to exactly new_length, which might be more predictable but could be less efficient if the buffer often already has enough space. (jsut a suggestion, not blocking this PR)
Signed-off-by: Scott K Logan <logans@cottsay.net>
|
4 participants
The logic upon which this algorithm is based is derived from recommended practices on an MSDN blog.
As it stands, quotes on arguments are lost when the argument array is converted to a single command line string. I can't find any alternative windows API for invoking a process like this which takes arguments as an array.
This PR includes modifications to the tests to embed the path to the CMake executable. I needed an executable that took more arguments to validate the handling of spaces in arguments, and this is the only one I could think of that would be available on all platforms.
See https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way