CoatiSoftware / vs-sourcetrail

Visual Studio Extension to communicate with Sourcetrail and generate a JSON Compilation Database from a Visual Studio Solution
Apache License 2.0
117 stars 42 forks source link

Don't use -isystem for non-system include folders (by default) #22

Closed dakotahawkins closed 6 years ago

dakotahawkins commented 6 years ago

I found that include-what-you-use (and who knows what else, since IWYU itself relies on clang's built-in judgement) relies on -isystem to indicate which include directives are for system includes, so using -isystem for every include directory results in it spamming angled brackets all-over the place.

By default, we will use -isystem for platform include directories and -I for project-specific additional include directories. While the effect angle brackets and double quotes have on include paths is essentially implementation defined, this default should mimic the overwhelming convention and corresponding behavior in MSVC/clang/gcc.

Because there are some projects that mandate exclusive use of angle brackets for everything, a checkbox is available to enable the old behavior.

dakotahawkins commented 6 years ago

Made sure to add a test and tooltip/help as well.

mlangkabel commented 6 years ago

Thank you for your work on this. Did you test this approach in combination with IWYU on some real world project?

dakotahawkins commented 6 years ago

@mlangkabel Yes. A real-world (very large) test case is what prompted this.

mlangkabel commented 6 years ago

That sounds great! Unfortunately we cannot release a new version of the extension that includes this pull request right now, because Sourcetrail requires the compilation database to use -isystem flags. So including this pull request in a release would break the Sourcetrail compatibility of the extension.

And here comes the good news: We have scheduled a new release of Sourcetrail for week after the next! So we can update the compilation database compatibility of Sourcetrail for that release and merge this pull request afterwards.

I'll review your changes as soon as possible!

dakotahawkins commented 6 years ago

As long as it gets there eventually!

It might be neat (or necessary, depending on how far you want to go to support weird configurations out-of-the-box) to find a way to run files through clang's preprocessor so you can make sure includes get resolved. Maybe even resolve them with the most restrictive rules you can (e.g. using -iquote sometimes instead of -I).

-isystem should probably find something 100% of the time, but it might find the wrong thing if you have a project header with the same filename as a system header.

mlangkabel commented 6 years ago

I thought about this, too, but unning clang's preprocessor would take time and would slow down generating the compilation database a lot. So I don't think that this would be a feasible solution ;)

Visual Studio doesn't care if you use quotes or angle brackets for your include directives or if you specify your header search paths as includes or as aditional includes, So I think that most Visual Studio users won't know/care, too.

Therefore I think that it would be better if we'd make it the default behavior of the extension to use -isystem for all include paths while providing the option to opt out of this for more experienced users. So, what do you think about turning this checkbox on by default?

Furthermore I think we should change the checkbox' label to "Non-system Includes May Use Angle Brackets". This tells the user that they don't have to use angle brackets for all the non-system includes.

dakotahawkins commented 6 years ago

Visual Studio doesn't care if you use quotes or angle brackets for your include directives or if you specify your header search paths as includes or as additional includes, So I think that most Visual Studio users won't know/care, too.

Actually, there is a difference in behavior for quotes and angle brackets.

I'd probably be OK changing the default. The real problem is using clang tools that care about the difference between them, so as long as there's an option I think it's fine.

mlangkabel commented 6 years ago

Thanks for providing that link! So the actual difference between including with quotes and including with angle brackets is that the compiler would search parent (and grandparent, etc.) directories of the file using the include directive if the directive uses quotes.

I'll merge your pull request now and change the default value for the added option in an additional commit.