Differential D450

Fix problem with include paths with clang language parser
ClosedPublic
Actions

Authored by amccann on Oct 25 2015, 4:56 AM.

Details

Reviewers
mwolff
kfunk
Commits
R32:a77ddc0f7c95: Do not add empty paths to include arguments for clang parser
R32:cf2eb0948fb4: Fix problem with include paths with clang language parser
Summary

Do not add empty paths to include arguments for clang parser

This fixed an issue I had with a custom build project.

I don't yet understand why empty paths were being passed in to the AddIncludes function.

While there is likely a problem elsewhere inserting the empty path, it seems reasonable to have this method be robust to bad input from plugins.

clang parser was getting called with commandline that looked like: "-I", "-I/path/to/dir"
Files located in "/path/to/dir" were not being found. Removing the "-I" fixed the problem.

Test Plan

Not sure how to reproduce my situation currently, other than manually ensuring an empty path is set.

I did verify that clang compiler itself exhibits this behavior from the commandline.

Diff Detail

Repository
R32 KDevelop
Branch
fix_clang_include_paths_problem
Lint
No Linters Available
Unit
No Unit Test Coverage
amccann updated this revision to Diff 1046.Oct 25 2015, 4:56 AM
amccann retitled this revision from to Fix problem with include paths with clang language parser.
amccann updated this object.
amccann edited the test plan for this revision. (Show Details)
amccann added reviewers: kfunk, mwolff.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 25 2015, 4:56 AM
amccann added inline comments.Oct 25 2015, 5:10 AM
languages/clang/duchain/parsesession.cpp
97

If the spirit of this diff is acceptable,

I wondered if this code should check other conditions.

could test

path.exists()

and possibly

path.isDir()
andric added a subscriber: andric.Oct 25 2015, 3:04 PM
mwolff edited edge metadata.Oct 25 2015, 3:33 PM

I agree that it's a good idea to have the check in, but please rewrite the check slightly to simplify the code.

Also, I guess you should fix the custom project manager to not add empty paths.

languages/clang/duchain/parsesession.cpp
93

simplify, and reduce the indentation depth of the code below by instead writing:

if (url.isEmpty()) {
    continue;
}

const auto path = url.toLocalFile().toUtf8();
...
97

sure, you can add the isDir check if that helps, that only returns true if the path exists. But is it really bad to pass non-existing, or non-dir, paths to clang? I don't think so, thus we shouldn't need to do anything here?

mwolff requested changes to this revision.Oct 25 2015, 3:43 PM
mwolff edited edge metadata.
This revision now requires changes to proceed.Oct 25 2015, 3:43 PM
amccann updated this revision to Diff 1051.Oct 25 2015, 8:38 PM
amccann edited edge metadata.
  • Do not add empty paths to include arguments for clang parser
  • Address style concerns in parsesession.cpp
amccann added a comment.Oct 25 2015, 8:43 PM

switched to continue. (Funny, because thats what I originally had, but i've seen people not like use of 'continue' so went with the indentation..)

Decided to not add any isDir() / exists() checks. Agreed that clang should handle 'bad paths' OK.

Thinking is: the nature of the bug here isn't that a path was 'bad' but that an empty path was more of a syntax problem to clang's argument parser.

languages/clang/duchain/parsesession.cpp
97 ↗(On Diff #1051)

Agreed, not necessary.

mwolff accepted this revision.Oct 26 2015, 3:35 PM
mwolff edited edge metadata.

you should get a developer account: https://techbase.kde.org/Contribute/Get_a_Contributor_Account put me in as a mentor, then you can push all of your changes yourself :P

This revision is now accepted and ready to land.Oct 26 2015, 3:35 PM
amccann added a comment.Oct 28 2015, 6:09 PM

I have commit privs now. Thanks!

I'm assuming most of these diff's I've made are "bug fixes" and should go onto the 5.0 branch?

kfunk edited edge metadata.Oct 29 2015, 8:26 AM
In D450#8919, @amccann wrote:

I have commit privs now. Thanks!

I'm assuming most of these diff's I've made are "bug fixes" and should go onto the 5.0 branch?

Yes, please push them to the 5.0 branch.

mwolff edited edge metadata.Oct 29 2015, 11:33 AM
mwolff added a project: KDevelop.
amccann updated this revision to Diff 1106.Oct 29 2015, 10:00 PM

Do not add empty paths to include arguments for clang parser

Closed by commit R32:a77ddc0f7c95: Do not add empty paths to include arguments for clang parser (authored by amccann). · Explain WhyOct 29 2015, 10:01 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1106

View Options

languages/clang/duchain/parsesession.cpp

Loading...