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

Fix system header search paths on macOS for Objective-C++, too. #1193

Merged
merged 1 commit into from
Feb 23, 2019
Merged

Fix system header search paths on macOS for Objective-C++, too. #1193

merged 1 commit into from
Feb 23, 2019

Conversation

s4y
Copy link
Contributor

@s4y s4y commented Feb 22, 2019

I've been running into ycm-core/YouCompleteMe#303, even though ycmd has a workaround for it. It looks like the issue is just that I'm writing ObjC++ and that change only affects C++.

So, this change makes -x objective-c++ eligible, too.


This change is Reviewable

aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 22, 2019
…its own libclang."

This reverts commit 75a861f.

Reason for revert: It turns out this is an upstream issue. See:

- https://chromium-review.googlesource.com/c/chromium/src/+/1482557
- ycm-core/ycmd#1193

Original change's description:
> [Vim/YCM] Fix missing system headers when YCM was built with its own libclang.
> 
> Bug: 932667
> Change-Id: I2eb9965da1eb3f9ad4a5ce103cf6e32d35019e48
> Reviewed-on: https://chromium-review.googlesource.com/c/1476105
> Auto-Submit: Sidney San Martín <[email protected]>
> Commit-Queue: Sidney San Martín <[email protected]>
> Commit-Queue: Asanka Herath <[email protected]>
> Reviewed-by: Asanka Herath <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#632846}

[email protected],[email protected]

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 932667
Change-Id: I5a684c91723d28470e1d903cd74674ce145b421b
Reviewed-on: https://chromium-review.googlesource.com/c/1483709
Reviewed-by: Sidney San Martín <[email protected]>
Commit-Queue: Sidney San Martín <[email protected]>
Cr-Commit-Position: refs/heads/master@{#634829}
This change just augments 4993964 to
apply to `-x objective-c++` as well as `-x c++` files.
@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #1193 into master will decrease coverage by 0.81%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
- Coverage   97.28%   96.46%   -0.82%     
==========================================
  Files          95       95              
  Lines        7108     7109       +1     
==========================================
- Hits         6915     6858      -57     
- Misses        193      251      +58

@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #1193 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   97.28%   97.28%   +<.01%     
==========================================
  Files          95       95              
  Lines        7108     7109       +1     
==========================================
+ Hits         6915     6916       +1     
  Misses        193      193

@s4y
Copy link
Contributor Author

s4y commented Feb 22, 2019

Hmm, I think the CI failure is unrelated to my change.

Copy link
Collaborator

@micbou micbou left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. :lgtm:

Hmm, I think the CI failure is unrelated to my change.

Yes, it was a flake.

Reviewed 2 of 2 files at r1.
Reviewable status: 1 of 2 LGTMs obtained

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Thanks for sending a PR!

Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained

@micbou
Copy link
Collaborator

micbou commented Feb 23, 2019

@zzbot r+

@zzbot
Copy link
Contributor

zzbot commented Feb 23, 2019

📌 Commit 832ee83 has been approved by micbou

@zzbot
Copy link
Contributor

zzbot commented Feb 23, 2019

⌛ Testing commit 832ee83 with merge 6d8ddd5...

zzbot added a commit that referenced this pull request Feb 23, 2019
… r=micbou

Fix system header search paths on macOS for Objective-C++, too.

I've been running into ycm-core/YouCompleteMe#303, even though ycmd has a workaround for it. It looks like the issue is just that I'm writing ObjC++ and that change only affects C++.

So, this change makes `-x objective-c++` eligible, too.

<!-- 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/1193)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Feb 23, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 6d8ddd5 to master...

@zzbot zzbot merged commit 832ee83 into ycm-core:master Feb 23, 2019
@s4y s4y deleted the objcpp-needs-fixed-header-search-paths-too branch March 4, 2019 20:30
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Mar 12, 2019
[READY] Update ycmd

Include the following changes:

 - PR ycm-core/ycmd#1180: trigger semantic completion when instructed by server;
 - PR ycm-core/ycmd#1184: simplify LSP completer API for starting server;
 - PR ycm-core/ycmd#1187: improve Clangd completer initialization;
 - PR ycm-core/ycmd#1191: ease Clangd completer initialization;
 - PR ycm-core/ycmd#1193: fix system header search paths on macOS for Objective-C++;
 - PR ycm-core/ycmd#1194: update Jedi to 0.13.3 and Parso to 0.3.4;
 - PR ycm-core/ycmd#1195: ignore identifiers returned by TSServer in JavaScript;
 - PR ycm-core/ycmd#1196: update TSServer to 3.3.3333;
 - PR ycm-core/ycmd#1197: support -B flag in C-family languages.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/3339)
<!-- Reviewable:end -->
SergioOpenPeer pushed a commit to webrtc-uwp/chromium-tools that referenced this pull request May 10, 2019
…its own libclang."

This reverts commit 75a861f213f0a3156743b68b5b3a060849abcfa4.

Reason for revert: It turns out this is an upstream issue. See:

- https://chromium-review.googlesource.com/c/chromium/src/+/1482557
- ycm-core/ycmd#1193

Original change's description:
> [Vim/YCM] Fix missing system headers when YCM was built with its own libclang.
> 
> Bug: 932667
> Change-Id: I2eb9965da1eb3f9ad4a5ce103cf6e32d35019e48
> Reviewed-on: https://chromium-review.googlesource.com/c/1476105
> Auto-Submit: Sidney San Martín <[email protected]>
> Commit-Queue: Sidney San Martín <[email protected]>
> Commit-Queue: Asanka Herath <[email protected]>
> Reviewed-by: Asanka Herath <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#632846}

[email protected],[email protected]

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 932667
Change-Id: I5a684c91723d28470e1d903cd74674ce145b421b
Reviewed-on: https://chromium-review.googlesource.com/c/1483709
Reviewed-by: Sidney San Martín <[email protected]>
Commit-Queue: Sidney San Martín <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#634829}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: a3e4d14604cf4403caed57da074a863910643141
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.

4 participants