Skip to content

Fixed a bug caused by paths with spaces in Windows.#51

Open
nicber wants to merge 2 commits into
rhysd:masterfrom
nicber:fix-path-windows
Open

Fixed a bug caused by paths with spaces in Windows.#51
nicber wants to merge 2 commits into
rhysd:masterfrom
nicber:fix-path-windows

Conversation

@nicber

@nicber nicber commented Oct 1, 2016

Copy link
Copy Markdown

If I specify
g:clang_format='"C:/Program Files (x86)/LLVM/bin/clang-format.exe"' (For example)
then the call to executable() fails because the executable function gets confused by the quotes, which
are necessary to escape the spaces.
This patch removes those quotes before testing if it is executable.

Comment thread autoload/clang_format.vim Outdated
" but executable() get confused by the quotes, so we remove them
if has("win32") && !executable(split(g:clang_format#command, '"')[0])
return 1
elseif !has("win32") && !executable(g:clang_format#command)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Conditions of if are complicated. I think it's good to separate them to multiple if statements
  • Indentation looks collapsed
if has('win32') || has('win64')
    if !executable(split(...))
        return 1
    endif
elseif !executable(g:clang_format#command)
    return 1
endif

@nicber nicber Oct 2, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed these in the latest commit.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rhysd

rhysd commented Oct 2, 2016

Copy link
Copy Markdown
Owner

I think quoting command name is not good. Does escaping white spaces such as some\ dir work well?

@nicber

nicber commented Oct 2, 2016

Copy link
Copy Markdown
Author

Unfortunatelly, escaping whitespace with \ does not work.

@rhysd

rhysd commented Oct 2, 2016

Copy link
Copy Markdown
Owner

I noticed that this is a problem when the command is run. I'll commit a fix for it tomorrow. Could you try it before merging this PR?

@nicber

nicber commented Oct 2, 2016

Copy link
Copy Markdown
Author

Sure, I'll test it.
My clang-command is '"C:/Program Files (x86)/LLVM/bin/clang-format.exe"'
I investigated this issue and found the following:

  • Quoting the path is necessary for system() to work.
  • Quoting makes the call to executable() fail.
  • If I don't quote the path, the failures are inversed.
    So, the solution I found was to remove the quotes before calling executable.

@rhysd

rhysd commented Oct 3, 2016

Copy link
Copy Markdown
Owner

@nicber

Please try latest with below config (removing quotes)

let g:clang_format#command = 'C:/Program Files (x86)/LLVM/bin/clang-format.exe'

@nicber

nicber commented Oct 3, 2016

Copy link
Copy Markdown
Author
Error detected while processing function clang_format#replace[2]..<SNR>45_verify_command[1]..clang_format#is_invalid:
line   10:
E684: list index out of range: 0
E15: Invalid expression: v[0] < 3 || (v[0] == 3 && v[1] < 4)
Error detected while processing function clang_format#replace[42]..<SNR>45_error_message:
line    1:
clang-format has failed to format.       

I get this error with the latest commit.

@rhysd

rhysd commented Oct 4, 2016

Copy link
Copy Markdown
Owner

Thank you for the report. It looks to resolve the quoting problem on Windows 😄 And it seems to fail to check clang version. I'll fix version check more strict. Could you show the output of clang-format.exe --version?

rhysd added a commit that referenced this pull request Oct 5, 2016
@rhysd

rhysd commented Oct 5, 2016

Copy link
Copy Markdown
Owner

Could you try the latest? (and please let me know the output of clang-format --version to fix skipping version check)

@nicber

nicber commented Oct 5, 2016

Copy link
Copy Markdown
Author

Sorry for the delay
I get this error with latest

Error detected while processing function clang_format#replace[42]..<SNR>45_error_message:
line    1:
clang-format has failed to format.   

This is the result of clang-format --version

C:\Users\nicol>"C:/Program Files (x86)/LLVM/bin/clang-format.exe" --version
clang-format version 3.9.0 (branches/release_39)

@rhysd

rhysd commented Oct 5, 2016

Copy link
Copy Markdown
Owner

Thank you for confirmation. Hmm... I think clang-format command failed by some reason. I'll investigate it on my Windows machine (I actually don't have Windows machine for development...)

Please ensure that your code can be formatted correctly from command line.

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.

2 participants