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

include pathes provided by isystem are ignored by compile commands parser #1156

Closed
wcscmp opened this issue Oct 23, 2017 · 12 comments
Closed
Labels
bug fixed Check the Milestone for the release in which the fix is or will be available. Language Service
Milestone

Comments

@wcscmp
Copy link

wcscmp commented Oct 23, 2017

No description provided.

@sean-mcmanus
Copy link
Collaborator

What is isystem? When your compileCommands is set, the includePath setting is intentionally ignored.

@sean-mcmanus sean-mcmanus added the more info needed The issue report is not actionable in its current state label Oct 23, 2017
@wcscmp
Copy link
Author

wcscmp commented Oct 23, 2017

https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html
-isystem is used for include from which we want to ignore warnings.
For example for third party libraries which we would not modify.

@sean-mcmanus
Copy link
Collaborator

sean-mcmanus commented Oct 23, 2017

How are you generating the compile_commands.json? Is this a bug with how we're processing that file or how it's generated by another tool? Are we not reading a particular field in the json?

@wcscmp
Copy link
Author

wcscmp commented Oct 24, 2017

I'm generating it using cmake with a -DCMAKE_EXPORT_COMPILE_COMMANDS=ON parameter provided.
In a cmake file target_include_directories is called with SYSTEM specified.
https://cmake.org/cmake/help/v2.8.12/cmake.html#command:target_include_directories
In a compile_commands.json include directories are provided in a "-isystem " format instead of "-I".
Headers from those directories cannot be found by cpptools.
I'm replacing "-isystem " with "-I" in compile_commands.json which resolves my issue. This is a manual step thought which is not nice.

@bobbrow
Copy link
Member

bobbrow commented Oct 24, 2017

We can fix this. Thanks for letting us know about the issue.

@bobbrow bobbrow added bug Language Service and removed more info needed The issue report is not actionable in its current state labels Oct 24, 2017
@mbodmer
Copy link

mbodmer commented Nov 21, 2017

Hi, I have the same issue here. I think you could just treat -isystem paths the same as normal -I include paths but do not warn if it is not possible to resolve any further includes from the referenced system header.

@bobbrow bobbrow added this to the December 2017 milestone Nov 27, 2017
@bobbrow bobbrow added the fixed Check the Milestone for the release in which the fix is or will be available. label Nov 27, 2017
@bobbrow
Copy link
Member

bobbrow commented Dec 12, 2017

This should be fixed in 0.14.4

@bobbrow bobbrow closed this as completed Dec 12, 2017
@niosHD
Copy link

niosHD commented Dec 12, 2017

Sorry to rain on your parade, but the problem is (at least for cmake generated compilation databases) unfortunately not fixed in 0.14.4.

I put together a minimal working example which shows the problem. In summary, it seems to be that -isystem is now handled exactly like -I meaning that the parser is expecting arguments like -I/path/to/directory and -isystem/path/to/directory. For -I that works since cmake generates the arguments in this form. However, for -isystem, cmake is generating the entries in the compilation database as -isystem /path/to/directory (note the space between -isystem and the path) which is not understood by the cpptools.

The result is that all -isystem paths with space are still ignored which puts us into exactly the same space before. I think it would in general be a good idea to support both forms (with and without space between the flag and the path) for -I and -isystem since gcc and clang also accept them.

@sean-mcmanus
Copy link
Collaborator

sean-mcmanus commented Dec 12, 2017

@niosHD Thanks for reporting this. We'll track this bug with #1343 . I'm not sure how @bobbrow tested this change if -isystem always has the space after it...oh, looks like our test uses a hand-generated compile_commands.json without the space.

@bobbrow
Copy link
Member

bobbrow commented Dec 13, 2017

I implemented the change based on my reading of the Clang documentation which shows no space after -isystem. We can support both forms though.

@sean-mcmanus
Copy link
Collaborator

@niosHD 0.14.5 should work with -isystem now (with a space). Let us know if you hit any other issues (preferably by filing a new bug).

@niosHD
Copy link

niosHD commented Dec 19, 2017

Perfect, I can confirm that the minimal working example works with 0.14.5 as expected. A quick test on a more complex project setup also looks promising.

@sean-mcmanus @bobbrow Thank you for the prompt fix!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fixed Check the Milestone for the release in which the fix is or will be available. Language Service
Projects
None yet
Development

No branches or pull requests

5 participants