yujinakayama / atom-lint

Obsolete: Generic code linting support for Atom
https://atom.io/packages/atom-lint
MIT License
111 stars 33 forks source link

Add C family linting with clang #40

Closed wesbland closed 10 years ago

wesbland commented 10 years ago

There currently isn't any linter here that works with the C family of libraries. This adds support for clang. It also add two new configuration options: the standard "path", and an "includes" option so the user can pass in include directories if necessary.

wesbland commented 10 years ago

Thanks for the great review. Sorry for my Coffee/Javascript noobishness. Hopefully this is more useful now.

yujinakayama commented 10 years ago

The commit message is still mentioning C#.

wesbland commented 10 years ago

Fixed.

On Tue, Apr 1, 2014 at 10:29 AM, Yuji Nakayama notifications@github.comwrote:

The commit message is still mentioning C#.

Reply to this email directly or view it on GitHubhttps://github.com/yujinakayama/atom-lint/pull/40#issuecomment-39219168 .

wesbland commented 10 years ago

During my last fix, I squash together two lines of output in the case where it's something like this:

In file included from /Users/wbland/Repositories/mpich/src/binding/fortran/use_mpi_f08/wrappers_c/address_cdesc.c:10: /Users/wbland/Repositories/mpich/src/binding/fortran/use_mpi_f08/wrappers_c/cdesc.h:12:10: fatal error: 'ISO_Fortran_binding.h' file not found

The second line is parsed and displayed correctly. However, the first line is not parsed (nor do I expect it to be), but it is pushed into the message for the second line. The result of this is that the text hangs over the tooltip border by quite a bit if the path is long (as in the example above). I assume the problem lies in a calculation somewhere in violation-view.coffee, but I think it's best if I don't try to fix things that I know nothing about.

yujinakayama commented 10 years ago

I think the In file included from ... line should not be pushed into the next line since the first line does not give us any helpful information. Also I don't like the too specific workaround that handles only In file included from.

yujinakayama commented 10 years ago

With the In file included from ... workaround:

screen shot 2014-04-02 at 0 56 37

Without the workaround:

screen shot 2014-04-02 at 0 57 48

I think the latter is much better.

wesbland commented 10 years ago

Fair enough. I thought it was a little misleading since you can really daisy chain yourself when going back through all of the headers to find the offending one, but if you prefer the latter, I don't really care. I'll take it out.

On Tue, Apr 1, 2014 at 11:01 AM, Yuji Nakayama notifications@github.comwrote:

With the In file included from ... workaround:

[image: screen shot 2014-04-02 at 0 56 37]https://cloud.githubusercontent.com/assets/83656/2581275/a1c0ca3c-b9b6-11e3-93f9-558f7a7c2a3f.png

Without the workaround:

[image: screen shot 2014-04-02 at 0 57 48]https://cloud.githubusercontent.com/assets/83656/2581277/ae302fb0-b9b6-11e3-8a77-3ffe54d79306.png

I think the latter is much better.

Reply to this email directly or view it on GitHubhttps://github.com/yujinakayama/atom-lint/pull/40#issuecomment-39223337 .

wesbland commented 10 years ago

Also, I think you were a little lucky there. The line/character number that it's pointing to is actually in the other header (not the file that you're currently viewing). On my system it looks much different:

screen shot 2014-04-01 at 11 08 36 am

That's why I originally chose to have it just point to 1,1

wesbland commented 10 years ago

I think my preference would be to remove the special case for In file included from and have the actual error point to 1,1 instead since there's no way to tell exactly where the error should point without some complex parsing.

yujinakayama commented 10 years ago

Hmm, I've tried on several sources (including both C and Obj-C) it properly points the location.

$ clang -v
Apple LLVM version 5.1 (clang-503.0.38) (based on LLVM 3.4svn)
Target: x86_64-apple-darwin13.1.0
Thread model: posix

I still prefer keeping the original location since it's not responsibility of atom-lint to handle it and it should be fixed in clang itself if it's really a bug.

wesbland commented 10 years ago

If you look at my example above:

In file included from /Users/wbland/Repositories/mpich/src/binding/fortran/use_mpi_f08/wrappers_c/address_cdesc.c:10:
/Users/wbland/Repositories/mpich/src/binding/fortran/use_mpi_f08/wrappers_c/cdesc.h:12:10: fatal error: 'ISO_Fortran_binding.h' file not found

The "correct" line is embedded in the first line (the special case). This line doesn't include a character number or error message. The second line tells you where the problem is in the included header, but it's not the correct location in the current file. I'm not sure what your version of clang is telling you that's turning out to be correct. We're running the same exact version of clang so that's not the issue. Can you show me your output?

yujinakayama commented 10 years ago
In file included from /Users/nkymyj/Projects/NAKPlaybackIndicatorView/Classes/NAKPlaybackIndicatorContentView.m:9:
/Users/nkymyj/Projects/NAKPlaybackIndicatorView/Classes/NAKPlaybackIndicatorContentView.h:9:9: fatal error: 'UIKit/UIKit.h' file not found
wesbland commented 10 years ago

Exactly. Your .m file just happened to have the header on line 9, character 9, where your header also had the offending header on line 9, character 9. If you put in an extra newline, I'm sure it will come out differently.

yujinakayama commented 10 years ago

I understood. If I insert some lines to the beginning of NAKPlaybackIndicatorContentView.h, atom-lint highlights wrong location.

yujinakayama commented 10 years ago

Then, I think we should parse the In file included from ... line and extract the line number so that atom-lint can highlight the line (though there's no column number). I've found fly-check does the same thing.

Sorry for throwing you into confusion. I'll implement that if you're tired.

wesbland commented 10 years ago

No problem. I'll take care of it.

wesbland commented 10 years ago

It should be fixed up now. It will ignore all but the first message about an include since the first one is the only one that applies here. I also added a bit more documentation for posterity since this became a bit of a mess.

yujinakayama commented 10 years ago

Thanks!