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

only skip flag that is actually a file #602

Closed
wants to merge 1 commit into from

Conversation

comicfans
Copy link

@comicfans comicfans commented Sep 17, 2016

this improves compatibility with msvc-cl style compile flags


This change is Reviewable

@comicfans
Copy link
Author

Hi, I'm trying to make YouCompleteMe works with MSVC style arguments better, these arguments starts with '/' but currently YouCompleteMe thinks these're file flag and throws them, this patch add a addition check to make sure arguments to throw is actually a file argument ,not MSVC style argument

@puremourning
Copy link
Member

Thanks for sending a PR!

We will need tests updated for this. I don't think we have ever tested MSVC style flags and tbh I didn't realise clang accepted them.

So I think this is a larger job :/


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@vheon
Copy link
Contributor

vheon commented Sep 17, 2016

I'm not sure I understood the problem, but this for sure needs a test to be written.

@micbou
Copy link
Collaborator

micbou commented Sep 17, 2016

Libclang only accepts MSVC style flags when the driver mode is set to cl (--driver-mode=cl flag). So, I think a better approach would be to look at the --driver-mode flag and accept MSVC flags (flags starting with a /) only if its value is cl.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@Valloric
Copy link
Member

Agreed with @micbou. Accepting CL-style flags is not a common case for clang. The drawback of looking for --driver-mode is that it might change in the future, although that's probably unlikely.

Either way, extensive tests are necessary.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@comicfans
Copy link
Author

I've created a modified msbuild to generate compile_commands.json for existing MSVC project and adds --driver-mode=cl to every compile commands, makes this work .
I think even looking for --driver-mode, we still need to determine if flag is file or not (cl mode with unix-like file path),so this condition check may be still needed.

@micbou
Copy link
Collaborator

micbou commented Sep 18, 2016

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


ycmd/completers/cpp/flags.py, line 312 at r1 (raw file):

          ( not previous_flag_starts_with_dash or
            ( not previous_flag_is_include and '/' in flag ) ) 
          and is_file):

This is wrong. We are supposed to ignore any file, not just the one currently edited.


Comments from Reviewable

@comicfans
Copy link
Author

is_file does not mean the one currently edited, but every flag in flags iterates.

@micbou
Copy link
Collaborator

micbou commented Sep 18, 2016

In _RemoveUnusedFlags function, filename is the file being passed to Clang so is_file is True if the flag points to the same path as the current file, False otherwise. What you want (at least if I understand what you are trying to achieve) is to be True for any file. That's not the same thing.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@comicfans
Copy link
Author

Yes you're right, I misuse this. what I want to achiveve is make sure it to be true for any file. so should I correct this , or waiting YCM implements --driver-mode=cl logic ?

this improves compatibility with msvc-cl style compile flags
@codecov-io
Copy link

Current coverage is 83.75% (diff: 0.00%)

Merging #602 into master will decrease coverage by 9.66%

@@             master       #602   diff @@
==========================================
  Files            41         41          
  Lines          3835       3836     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3583       3213   -370   
- Misses          252        623   +371   
  Partials          0          0          

Powered by Codecov. Last update 1a82d7f...c269a21

@micbou
Copy link
Collaborator

micbou commented Dec 8, 2017

Closed in favor of #789.

@micbou micbou closed this Dec 8, 2017
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.

6 participants