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

Parse more C/C++ compiler options #2131

Merged
merged 1 commit into from
Jan 26, 2019
Merged

Parse more C/C++ compiler options #2131

merged 1 commit into from
Jan 26, 2019

Conversation

0mco
Copy link
Contributor

@0mco 0mco commented Dec 9, 2018

According gcc manual, in addition to '-I/path/to/include', there are many other flags can be used to add header search directory. Qt with cmake use '-isystem' to include Qt headers, if not adding these flags to cflag_line, ale cannot find Qt headers. This PR adds these flags to cflag_line.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Could be good. Add tests for all of these flags.

@0mco
Copy link
Contributor Author

0mco commented Dec 10, 2018

Well, adding these flags to c linter solves the header issue, but it also exposes another problem:
After adding this, I got error:

In file included from /home/***/Applications/Qt/5.10.1/gcc_64/include/QtGui/qtguiglobal.h:43:
/home/***/Applications/Qt/5.10.1/gcc_64/include/QtCore/qglobal.h:1162:4: error: "You must build your code with position independent code if Qt was built with -reduce-relocations. "         "Compile your code with -fPIC (-fPIE is not enough)."
#  error "You must build your code with position independent code if Qt was built with -reduce-relocations. "\
   ^

The-fPICflag is in compile_commands.json, but it's not passed into c linter. Passing -fPIC to c linter solves this issue, but what about other potential (unknown) necessary flags?
Why ale only pass -D and -I flags to c linter? Is it for compatibility consideration? Since some flags may not be recognized by c linter?

@0mco
Copy link
Contributor Author

0mco commented Dec 10, 2018

Passing all flags found to c linter seems necessary, with the potential issue of 'unrecognized option'.

@w0rp
Copy link
Member

w0rp commented Dec 10, 2018

ALE only pases the flags it passes because that's all that has been implemented so far. Once someone implements more support for parsing the right flags, then they will work. There will probably be some flags we shouldn't pass, like -o for example.

@0mco
Copy link
Contributor Author

0mco commented Dec 11, 2018

Can you explain why we shouldn't pass -o option?
I've tested passing all flags to c linter, it seems working pretty well, even with deliberately wrong option like -asdfasdf, which c linter should throw 'unrecognized option' error, but it didn't.
I don't know how, but it seems the c linter ignores these options?

@w0rp
Copy link
Member

w0rp commented Dec 11, 2018

Yes. Say if a C or C++ complier passes an option like -o for generating build files, then running the compiler with that option will cause linting a file to leave binary files lying around in your project. If ALE did that, people would be understandably annoyed. -o itself might not cause that problem, but some other flags might cause that problem.

The problem is just trying to figure out which flags shouldn't be included, and how to exclude those flags. The way you implement that is by adding a large series of tests which capture all of the cases, such that there are no important cases which are not tested, and everything is covered.

@0mco
Copy link
Contributor Author

0mco commented Dec 13, 2018

@w0rp I found a summary of GCC options. I looked at Options Controlling the Kind of Output section, found there is actually only one option for controlling output, namely -o.
I've also tested with -o option, which made me sure that -o option is ignored automatically by the linter. This is great, since we don't have to distinguish which options should be passed into linter and which shouldn't.
Now that generating build files is not a problem any more, passing all options seems doable?

update: forget to mention, I only tested for clang.

@w0rp
Copy link
Member

w0rp commented Dec 16, 2018

Yeah, if it works, try it. All that matters is that relatively accurate output is produced, and we don't generate binary files or execute code. Being able to delete the messy code for parsing the flags would be nice bonus.

@0mco 0mco closed this Dec 21, 2018
@0mco 0mco reopened this Dec 21, 2018
@0mco 0mco changed the title add other C/C++ include flags from compile_commands.json Parse more C/C++ compiler options Dec 21, 2018
@0mco
Copy link
Contributor Author

0mco commented Dec 21, 2018

@w0rp Passing all options to clang seems to work pretty well, but gcc is bad at handling conflict options. In fact, even -c and -o will cause conflicts:

<<<OUTPUT STARTS>>>
gcc: fatal error: cannot specify -o with -c, -S or -E with multiple files
compilation terminated.
<<<OUTPUT ENDS>>>

So I passed all but -o option to linter. If this is ok, I'll add tests for this later.

@0mco
Copy link
Contributor Author

0mco commented Dec 21, 2018

There may be other conflicting options in gcc, which will only be addressed when we know that since I cant find a list of conflicting options for gcc.

@w0rp
Copy link
Member

w0rp commented Dec 25, 2018

Sure, go ahead and update the tests.

@w0rp
Copy link
Member

w0rp commented Jan 3, 2019

Okay, I merged a PR for problems with minuses from another guy. Update the code and the tests here, so everything passes now.

@w0rp
Copy link
Member

w0rp commented Jan 5, 2019

The tests failed, so have a look at those.

@w0rp
Copy link
Member

w0rp commented Jan 8, 2019

I just merged a PR for caching, so you'll need to rebase your commits here. It looks like you used git merge before. You might want to rewrite all of your changes into one commit.

Fix the tests here.

@0mco
Copy link
Contributor Author

0mco commented Jan 10, 2019

I'm preparing my final exams these days. I'll fix it later.

@w0rp
Copy link
Member

w0rp commented Jan 10, 2019

Sure thing. Thank you for trying to make this better!

@0mco 0mco force-pushed the master branch 2 times, most recently from ed54c52 to 3cc4324 Compare January 22, 2019 04:12
@0mco
Copy link
Contributor Author

0mco commented Jan 24, 2019

@w0rp I've updated.

@w0rp w0rp merged commit 6e9040d into dense-analysis:master Jan 26, 2019
@w0rp
Copy link
Member

w0rp commented Jan 26, 2019

Cheers! 🍻

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.

None yet

2 participants