-
Notifications
You must be signed in to change notification settings - Fork 771
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
[READY] Enable windows style flags when --driver-mode=cl is found in flags #789
Conversation
Codecov Report
@@ Coverage Diff @@
## master #789 +/- ##
==========================================
+ Coverage 96.27% 96.28% +<.01%
==========================================
Files 84 84
Lines 6504 6519 +15
==========================================
+ Hits 6262 6277 +15
Misses 242 242 |
Thanks for the PR. Reviewed 2 of 2 files at r1. ycmd/completers/cpp/flags.py, line 406 at r1 (raw file):
This variable should be set before looping through the flags, otherwise, Windows flags defined before enable_windows_style_flags = '--driver-mode=cl' in flags or if we want to be more correct, we should find the last occurrence of ycmd/completers/cpp/flags.py, line 433 at r1 (raw file):
This doesn't keep the order of flags which I think is important. If Comments from Reviewable |
3e61b03
to
1581d48
Compare
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions. ycmd/completers/cpp/flags.py, line 406 at r1 (raw file): Previously, micbou wrote…
Done. I included the "more correct" checking as well. ycmd/completers/cpp/flags.py, line 433 at r1 (raw file): Previously, micbou wrote…
Done. It occured to me that it was not preserving order though. Comments from Reviewable |
Reviewed 1 of 1 files at r2. ycmd/completers/cpp/flags.py, line 428 at r2 (raw file):
This can be simplified to: not enable_windows_style_flags or os.path.exists( flags ) Comments from Reviewable |
1581d48
to
9e181f7
Compare
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion. ycmd/completers/cpp/flags.py, line 428 at r2 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion. ycmd/completers/cpp/flags.py, line 397 at r3 (raw file):
Why don't we just use So I don't see the benefit of the We could look at the last Comments from Reviewable |
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion. ycmd/completers/cpp/flags.py, line 397 at r3 (raw file): Previously, Valloric (Val Markovic) wrote…
If we used If we do the Comments from Reviewable |
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion. ycmd/completers/cpp/flags.py, line 397 at r3 (raw file): Previously, bstaletic wrote…
Ah, you're right, my bad! Make sure to add a test for this; I can totally see someone making the same mistake in the future. Comments from Reviewable |
with a test for the Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion. Comments from Reviewable |
9e181f7
to
359ac96
Compare
359ac96
to
02a2942
Compare
I found a bug in the new code. I'll be debugging this tonight. Once again, sorry for dragging on forever what should have been a quick PR. Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion. Comments from Reviewable |
Whats the status of this PR? Any help needed? |
Here's the problem.
If we change the function to: def _RemoveFlagsPrecedingCompiler( flags ):
"""Assuming that the flag just before the first flag (which starts with a
dash) is the compiler path, removes all flags preceding it."""
for index, flag in enumerate( flags ):
if flag.startswith( '-' ) or ( flag.startswith( '/' ) and not os.path.isfile( flag ) ):
return ( flags[ index - 1: ] if index > 1 else
flags )
return flags[ :-1 ] Then the fuction would not strip too much. So far so good. So I gave it a fight two days in a row. And every time I fixed something five other things would break. |
OK I'll have a look at it because for me it would be very helpful to use YouCompleteme with clang-cl driver mode. |
feb5318
to
fcbbacd
Compare
Finally made this work, as far as I can tell. Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions. ycmd/completers/cpp/flags.py, line 397 at r3 (raw file): Previously, Valloric (Val Markovic) wrote…
I believe the provided tests are enough to prevent this mistake from happening. ycmd/completers/cpp/flags.py, line 577 at r4 (raw file):
My current solution simply does the following: INCLUDE_FLAGS_WIN_STYLE = [ '/I' ]
PATH_FLAGS = [ '--sysroot=' ] + INCLUDE_FLAGS + INCLUDE_FLAGS_WIN_STYLE Comments from Reviewable |
fcbbacd
to
0eaf64e
Compare
@bstaletic Thank you so much. I won't have time to play with this until the 8th but I'll report back. |
@crapp I'll look forward to your review. Might be the first real world test of this. Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions. Comments from Reviewable |
I realise this is still RFC, but I feel like there are now a bunch of special cases where we probably need more tests. Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions. ycmd/completers/cpp/flags.py, line 577 at r4 (raw file): Previously, bstaletic (Boris Staletic) wrote…
That seems reasonable, though a more complete solution would be: path_flags = [ PATH_FLAGS ]
if enable_windows_style_flags:
path_flags.extend( WINDOWS_PATH_FLAGS )
for path_flag in path_flags: I guess in theory there is a chance that when driver mode is not ycmd/completers/cpp/flags.py, line 118 at r5 (raw file):
It feels strange that Then again, it doesn't look like ycmd/completers/cpp/flags.py, line 161 at r5 (raw file):
why do we store this as a member ? ycmd/completers/cpp/flags.py, line 326 at r5 (raw file):
probably needs updating ycmd/completers/cpp/flags.py, line 354 at r5 (raw file):
does this need to to check for windows flags too ? e.g. Comments from Reviewable |
I agree that the code feels clumsy and isn't very pretty to look at. And I'm being nice to myself and this code I've written. Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions. ycmd/completers/cpp/flags.py, line 577 at r4 (raw file):
That's correct, but I would argue I was thinking of propagating the above ycmd/completers/cpp/flags.py, line 118 at r5 (raw file): Previously, puremourning (Ben Jackson) wrote…
Now that I think of it, yes this could just return ycmd/completers/cpp/flags.py, line 161 at r5 (raw file): Previously, puremourning (Ben Jackson) wrote…
Because that's the first thing that came to my mind after thinking "I'll need this boolean in more than one method". If you have a better idea, I'm listening. ycmd/completers/cpp/flags.py, line 326 at r5 (raw file): Previously, puremourning (Ben Jackson) wrote…
My idea was:
It did occur to me that this might be wrong now. ycmd/completers/cpp/flags.py, line 354 at r5 (raw file): Previously, puremourning (Ben Jackson) wrote…
I think not. Definitely not how you have written it. Here's why.
If we do it like you suggested you we would break the case where Comments from Reviewable |
No worries, take your time.
Yes, and you should be able to do |
I hacked your patch to just simply return true just to be safe. It... sort of works... The flags look correct now in YcmDebugInfo, but I'm getting "ycm_core.ClangParseError: An AST deserialization error occurred while parsing the translation unit." |
Are you using MSVC 2017 ? Current libclang has problems with the latest MSVC standard library headers, leading to this problem. The only solution at the moment is to build a trunk clang. It might be resolved with clang 6 in the next month or so. |
@bstaletic It seems like your patch works, for sure. The flags are correct. @puremourning Ah that might be why. I'm upgrading to MSVC 2017 as we speak. |
@puremourning Wait, apologies- did you mean I should use MSVC 2017 to compile ycmd? Or do you mean that my project should use the STL that comes with MSVC 2017? |
I mean that if your project uses the standard library in the latest MSVC, you'll get these errors from ycmd (or any other libclang-based parser using the latest released libclang). This is because libclang 5.0.1 (the latest release version) chokes on the MSVS standard library headers for some reason (I don't know why). It's fixed in upstream clang trunk, and might be fixed in clang 6 (and thus libclang 6). One solution might be to build clang from source (either clang 6 release candidate, or trunk) and use the YCM "full installation guide" to rebuild ycmd pointing at that build of clang. |
@puremourning Ah. In that case, I'm still using MSVC 14 2015, not 2017. Does that version of the STL have this issue? |
Do you get the error each time you are trying to complete or it only happens once? |
Well, it would appear that libclang is choking on your code or the STL, though I haven't heard it reported against MSVC 2014. But I don't use Windows or MSVC, so it's difficult for me to check. It's fair to say that the cl driver mode stuff in libclang is fairly new and so some issues are to be expected. I would recommend trying the upstream libclang trunk, as others have reported it to be more stable in this mode, though again YMMV :/ |
@micbou It looks like it happens every time I try to complete. @puremourning Hmmm... well... I may have the rest of the afternoon cut out for me then, that sounds involved. |
@micbou To be clear- when I use a STL vector and try to dot-complete by typing a dot in insert mode in Vim, nothing happens. When I press escape immediately after, I see a red status in vim: "ClangParseError: An AST deserialization error occurred while parsing the translation unit." |
Building LLVM/clang isn't difficult, just time consuming. Don't forget to |
@puremourning Yeah, I think I'll give it a try. Even when it works, it's... weird. It was complaining about a signature mismatch for a method that definitely wasn't mismatched. The signature involved a vector. I restarted ycmd and got the AST error again. It may take awhile... |
@puremourning I'm not sure if we should recognise While the above patch works, it's not very efficient. Here's a better one: diff --git a/ycmd/completers/cpp/flags.py b/ycmd/completers/cpp/flags.py
index 945a6af..0f1efad 100644
--- a/ycmd/completers/cpp/flags.py
+++ b/ycmd/completers/cpp/flags.py
@@ -244,9 +244,16 @@ def _ExtractFlagsList( flags_for_file_output ):
def _ShouldAllowWinStyleFlags( flags ):
enable_windows_style_flags = False
if OnWindows():
- for flag in flags:
+ # clang-cl implies --driver-mode=cl
+ enable_windows_style_flags = 'clang-cl' in flags[ 0 ]
+ # Iterate in reverse because we only care
+ # about the last occurance of --driver-mode flag.
+ # Even with clang-cl, if the last --driver-mode is 'gcc'
+ # we shouldn't enable win flags.
+ for flag in reversed( flags ):
if flag.startswith( '--driver-mode' ):
- enable_windows_style_flags = ( flag == '--driver-mode=cl' )
+ enable_windows_style_flags = flag == '--driver-mode=cl'
+ break
return enable_windows_style_flags
|
@bstaletic I'm not set up to do patches, but it looks like you should be able to skip checking the flags for --driver-mode=cl if you've detected we're using clang-cl already. There's no reason to allow somebody using clang-cl to disable windows-style flags, right? That's guaranteed to be broken, because clang-cl doesn't understand non-windows-stye flags. E.g.:
|
@joelfrederico |
@bstaletic Really?!? Wow... I was going off of the clang-cl help, I guess that's not documented there. |
Wait... if that's true, don't you want to reverse the sense of the test for the flags if |
Not really. Note that you can still do
That matches with the clang behaviour. |
@bstaletic Do you have a link to the clang behaviour? It seems to me that your example breaks my understanding of flags. At the very least, there's a logical inconsistency in your last post:
Your example finds clang-cl.
Your example has this flag.
This condition is met. But you said earlier that the last flag is a no-op. So we aren't in CL-enabled mode, but your algorithm thinks we are.
These two lines aren't reached. I mean, if you don't want to handle this edge-case, that's okay. I'm just a bit confused because this doesn't match the clang behavior you just described? |
No, I have a very simple test case which I'm relying on. You didn't understand what I meant by no-op. If you are using
This is where you are wrong. In the example above, MSVC flags are enabled, first with |
@bstaletic Ah but you said earlier:
But that's not the same as:
Which one of those is true?? Or maybe I'm missing a subtlety here?? |
Heh, in the first example I was thinking of
|
@bstaletic Okay, then everything makes perfect sense! That initial comment threw me off. I'm on board now! |
@puremourning Okay, so- I built libclang.dll. The documentation regarding linking against an externally-built libclang.dll for Windows seems to be wrong:
I'm fighting with LNK2001 errors. It seems like the final linking step of ycm_core isn't linking properly against libclang, or libclang didn't properly export symbols it needs. Edit: I compiled libclang.dll as a 32-bit binary, I needed a 64-bit binary. I missed the warning that got buried. Maybe that should be an error, but it's something I should've noticed a lot sooner. |
I'm sorry I have no experience of doing this on Windows, but my general approach is:
I realise that isn't an obvious way from the docs, but ultimately it boils down to using hope that helps. |
@puremourning I got it compiled, but I still get that AST deserialization error. I'm sort of ignoring it. The flags look fine to me and they work in some places, so this is a different error from flags (what's going on in this thread, right)? The right thing to do would be to figure out what my actual problem is (not Windows vs gcc flags) and open a separate issue, yes? |
[READY] Add clang-cl support According to #789 (comment), #789 didn't include support for `clang-cl` without `--driver-mode=cl` specified. I did not add `clang-cl` to the C++ regex. Since there is no `clang-cl++` I think we should not touch language flags and let clang decide if it's C or C++. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/936) <!-- Reviewable:end -->
Windows (cl) style flags are currently filtered and thus windows users are unable to use a
.ycm_extra_conf.py
such as this one:If
--driver-mode=cl
is in flags, libclang gladly accepts these flags. This pull request checks for--driver-mode=cl
before filtering the flags. U used the above.ycm_extra_conf.py
to confirm if everything works. The resulting:YcmDebugInfo
:Fixes ycm-core/YouCompleteMe#2491
This change is